From 312b58bef206dda4e1a3a4879b3b9132bfd1cafd Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:15:03 +0400 Subject: [PATCH 1/8] Test a pre-commit hook that runs make quality (failing) 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 --- .../accepted/pre-commit | 7 + .../rejected/pre-commit | 6 + tests/unit/test_pre_commit_hook.py | 304 ++++++++++++++++++ 3 files changed, 317 insertions(+) create mode 100755 rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit create mode 100755 rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit create mode 100644 tests/unit/test_pre_commit_hook.py diff --git a/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit b/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit new file mode 100755 index 000000000..f674b0ae6 --- /dev/null +++ b/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit @@ -0,0 +1,7 @@ +#!/bin/sh +# Proof fixture for api.quality.pre-commit-hook (accepted): the least a hook +# must do. It runs make quality, fails when make quality fails, and fails when +# make quality changed a file, whose fix still needs staging. +before=$(git diff) +make quality || exit 1 +[ "$(git diff)" = "$before" ] || exit 1 diff --git a/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit b/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit new file mode 100755 index 000000000..597cbb00a --- /dev/null +++ b/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit @@ -0,0 +1,6 @@ +#!/bin/sh +# Proof fixture for api.quality.pre-commit-hook (rejected): it runs make +# quality but lets the commit through whether the checks fail or ruff's fixes +# changed a file. +make quality +exit 0 diff --git a/tests/unit/test_pre_commit_hook.py b/tests/unit/test_pre_commit_hook.py new file mode 100644 index 000000000..4fcd9310a --- /dev/null +++ b/tests/unit/test_pre_commit_hook.py @@ -0,0 +1,304 @@ +"""A git pre-commit hook runs make quality (api.quality.pre-commit-hook). + +make install turns on .githooks/pre-commit with git config core.hooksPath +.githooks, which every worktree of the clone shares. The hook runs make +quality from the root of the working tree. It fails the commit when a check +fails, when ruff's fixes changed files that still need staging, or when make, +uv or the worktree's dev environment (.venv) is missing. These tests run it +in temporary repositories whose Makefile's quality target stands in for the +real checks. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parents[2] +HOOK = REPO / ".githooks" / "pre-commit" +FIXTURES = REPO / "rules" / "fixtures" / "api.quality.pre-commit-hook" +# Variables of make's own, and git settings outside the test repository, that +# the test's git and make mustn't inherit. +LEAKING_VARIABLES = frozenset( + {"MAKEFLAGS", "MFLAGS", "GNUMAKEFLAGS", "MAKEFILES", "MAKELEVEL", "GIT_DIR"} +) +STUB_UV = "#!/bin/sh\nexit 0\n" +PASSING_QUALITY = 'quality:\n\t@echo checked >> "$$QUALITY_MARKER"\n' +FAILING_QUALITY = "quality:\n\t@echo 'mypy: 1 error'\n\t@exit 1\n" +FIXING_QUALITY = "quality:\n\t@printf 'VALUE = 1\\n' > module.py\n" + + +def _environment(tmp_path: Path, *, with_uv: bool = True) -> dict[str, str]: + """Return an environment with a stand-in uv and no global git settings.""" + stubs = tmp_path / "stubs" + stubs.mkdir(exist_ok=True) + if with_uv: + (stubs / "uv").write_text(STUB_UV) + (stubs / "uv").chmod(0o755) + search_path = [str(stubs)] + for tool in ("git", "make", "sh"): + directory = str(Path(shutil.which(tool) or "/usr/bin").parent) + if directory not in search_path: + search_path.append(directory) + environment = { + name: value + for name, value in os.environ.items() + if name not in LEAKING_VARIABLES and not name.startswith("GIT_") + } + environment.update( + { + "PATH": os.pathsep.join([*search_path, "/usr/bin", "/bin"]), + "HOME": str(tmp_path), + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "test", + "GIT_AUTHOR_EMAIL": "test@example.com", + "GIT_COMMITTER_NAME": "test", + "GIT_COMMITTER_EMAIL": "test@example.com", + "QUALITY_MARKER": str(tmp_path / "quality-ran"), + } + ) + return environment + + +def _git( + cwd: Path, environment: dict[str, str], *arguments: str +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", "-c", "commit.gpgsign=false", *arguments], + cwd=cwd, + env=environment, + capture_output=True, + text=True, + check=False, + ) + + +def _repository( + tmp_path: Path, + environment: dict[str, str], + quality_recipe: str, + hook: Path = HOOK, + *, + dev_environment: bool = True, +) -> Path: + """Create a repository whose commits run ``hook``, with one commit made.""" + repository = tmp_path / "repository" + (repository / ".githooks").mkdir(parents=True) + shutil.copy(hook, repository / ".githooks" / "pre-commit") + (repository / ".githooks" / "pre-commit").chmod(0o755) + (repository / "Makefile").write_text(quality_recipe) + (repository / "module.py").write_text("VALUE = 0\n") + (repository / "other.py").write_text("OTHER = 0\n") + (repository / ".gitignore").write_text(".venv/\n") + if dev_environment: + (repository / ".venv").mkdir() + _git(repository, environment, "init", "--quiet", "--initial-branch=master") + _git(repository, environment, "add", ".") + _git(repository, environment, "commit", "--quiet", "-m", "start") + _git(repository, environment, "config", "core.hooksPath", ".githooks") + return repository + + +def _commit_a_change( + repository: Path, environment: dict[str, str] +) -> subprocess.CompletedProcess[str]: + (repository / "module.py").write_text("VALUE = 2\n") + _git(repository, environment, "add", "module.py") + return _git(repository, environment, "commit", "-m", "change") + + +def _commits(repository: Path, environment: dict[str, str]) -> int: + counted = _git(repository, environment, "rev-list", "--count", "HEAD") + return int(counted.stdout) + + +def hook_problems(hook: Path, tmp_path: Path) -> list[str]: + """Return how ``hook`` fails to stop a commit api.quality.pre-commit-hook stops.""" + problems = [] + for name, recipe, blocks in ( + ("a failing check", FAILING_QUALITY, True), + ("ruff's fixes to a staged file", FIXING_QUALITY, True), + ("passing checks", PASSING_QUALITY, False), + ): + scenario = tmp_path / name.replace(" ", "-").replace("'", "") + scenario.mkdir(parents=True) + environment = _environment(scenario) + repository = _repository(scenario, environment, recipe, hook) + result = _commit_a_change(repository, environment) + if blocks and result.returncode == 0: + problems.append(f"the hook lets a commit through after {name}") + if not blocks and result.returncode != 0: + problems.append(f"the hook stops a commit after {name}") + return problems + + +def test_rule_proof_pre_commit_hook(tmp_path: Path): + rejected = hook_problems(FIXTURES / "rejected" / "pre-commit", tmp_path / "bad") + accepted = hook_problems(FIXTURES / "accepted" / "pre-commit", tmp_path / "good") + + assert rejected == [ + "the hook lets a commit through after a failing check", + "the hook lets a commit through after ruff's fixes to a staged file", + ] + assert accepted == [] + + +def test_the_repository_s_hook_stops_what_it_should(tmp_path: Path): + assert hook_problems(HOOK, tmp_path) == [] + + +def test_a_commit_runs_make_quality(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + + result = _commit_a_change(repository, environment) + + assert result.returncode == 0, result.stderr + assert (tmp_path / "quality-ran").read_text() == "checked\n" + assert _commits(repository, environment) == 2 + + +def test_a_failing_check_stops_the_commit(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, FAILING_QUALITY) + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "make quality failed" in result.stderr + assert _commits(repository, environment) == 1 + + +def test_fixes_that_need_staging_stop_the_commit_and_are_named(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, FIXING_QUALITY) + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "module.py" in result.stderr + assert "stage" in result.stderr + assert _commits(repository, environment) == 1 + + +def test_unstaged_changes_make_quality_leaves_alone_dont_stop_the_commit( + tmp_path: Path, +): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "other.py").write_text("OTHER = 1\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode == 0, result.stderr + assert _commits(repository, environment) == 2 + + +def test_a_missing_uv_stops_the_commit_with_a_message(tmp_path: Path): + environment = _environment(tmp_path, with_uv=False) + if shutil.which("uv", path=environment["PATH"]): + pytest.skip("uv is installed in a system directory") + repository = _repository(tmp_path, environment, PASSING_QUALITY) + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "uv isn't installed" in result.stderr + assert "make install" in result.stderr + assert not (tmp_path / "quality-ran").exists() + + +def test_a_missing_dev_environment_stops_the_commit_with_a_message(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository( + tmp_path, environment, PASSING_QUALITY, dev_environment=False + ) + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "no dev environment" in result.stderr + assert "make install" in result.stderr + assert not (tmp_path / "quality-ran").exists() + + +def test_every_worktree_of_the_clone_runs_the_hook(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + worktree = tmp_path / "worktree" + _git( + repository, + environment, + "worktree", + "add", + "--quiet", + "-b", + "other", + str(worktree), + ) + (worktree / ".venv").mkdir() + (worktree / "module.py").write_text("VALUE = 3\n") + _git(worktree, environment, "add", "module.py") + + result = _git(worktree, environment, "commit", "-m", "change in a worktree") + + assert result.returncode == 0, result.stderr + assert (tmp_path / "quality-ran").read_text() == "checked\n" + + +def test_make_install_turns_the_hook_on(tmp_path: Path): + environment = _environment(tmp_path) + repository = tmp_path / "repository" + repository.mkdir() + shutil.copy(REPO / "Makefile", repository / "Makefile") + _git(repository, environment, "init", "--quiet") + + installed = subprocess.run( + ["make", "install"], + cwd=repository, + env=environment, + capture_output=True, + text=True, + check=False, + ) + + assert installed.returncode == 0, installed.stderr + hooks_path = _git(repository, environment, "config", "core.hooksPath") + assert hooks_path.stdout.strip() == ".githooks" + + +def test_make_install_outside_a_git_repository_still_installs(tmp_path: Path): + # A Docker build or a source archive has no repository to configure. + environment = _environment(tmp_path) + directory = tmp_path / "source" + directory.mkdir() + shutil.copy(REPO / "Makefile", directory / "Makefile") + environment["GIT_CEILING_DIRECTORIES"] = str(tmp_path) + + installed = subprocess.run( + ["make", "install"], + cwd=directory, + env=environment, + capture_output=True, + text=True, + check=False, + ) + + assert installed.returncode == 0, installed.stderr + + +def test_the_hook_is_executable_in_git(): + listed = subprocess.run( + ["git", "ls-files", "--stage", "--", ".githooks/pre-commit"], + cwd=REPO, + capture_output=True, + text=True, + check=True, + ).stdout + + assert listed.startswith("100755 ") From be86f111750aa956a16a455a7dc05382a7c6e598 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:17:11 +0400 Subject: [PATCH 2/8] Run make quality before every commit with a git pre-commit hook - .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 --- .githooks/pre-commit | 55 +++++++++++++++++++++++++++ AGENTS.md | 14 ++++--- Makefile | 5 +++ README.md | 5 ++- changelog.d/4206.added.md | 1 + docs/engineering/skills/github-prs.md | 16 +++++++- docs/generated/rules.md | 22 +++++++---- rules/registry.toml | 33 ++++++++++++++++ 8 files changed, 135 insertions(+), 16 deletions(-) create mode 100755 .githooks/pre-commit create mode 100644 changelog.d/4206.added.md diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 000000000..4acf3e011 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,55 @@ +#!/bin/sh +# The repository's pre-commit hook (api.quality.pre-commit-hook). It runs +# make quality before each commit, so the checks pull-request CI runs fail +# here first. make install turns it on with git config core.hooksPath +# .githooks; every worktree of the clone shares that setting and runs its own +# checkout's copy of this file. git commit --no-verify skips it, and CI's +# Quality checks job still runs the same checks. +# +# The commit stops when make quality fails, when ruff's fixes changed a file +# that still needs staging, or when make, uv or the worktree's .venv is +# missing. +set -u + +fail() { + printf 'pre-commit: %s\n' "$1" >&2 + exit 1 +} + +root=$(git rev-parse --show-toplevel) || + fail "can't find the root of this working tree." +cd "$root" || fail "can't enter $root." + +command -v make >/dev/null 2>&1 || + fail "make isn't installed, so make quality can't run. Install make, then commit again." +command -v uv >/dev/null 2>&1 || + fail "uv isn't installed, so make quality can't run. Install uv (https://docs.astral.sh/uv/), run make install, then commit again." +[ -d .venv ] || + fail "this worktree has no dev environment (.venv). Run make install here, then commit again." + +# Each tracked file with changes not staged for this commit, with a hash of +# its content. A line that differs after make quality is a file it changed. +unstaged_files() { + git diff --name-only | while IFS= read -r path; do + if [ -f "$path" ]; then + printf '%s %s\n' "$(git hash-object -- "$path")" "$path" + else + printf 'deleted %s\n' "$path" + fi + done +} + +before=$(mktemp) || fail "can't create a temporary file." +after=$(mktemp) || fail "can't create a temporary file." +trap 'rm -f "$before" "$after"' EXIT + +unstaged_files >"$before" +make quality || + fail "make quality failed. Fix what it reported above, then commit again." +unstaged_files >"$after" + +changed=$(grep -Fxv -f "$before" "$after" | sed 's/^[^ ]* //') +if [ -n "$changed" ]; then + printf 'pre-commit: make quality changed these files. Review the fixes, stage them, then commit again:\n%s\n' "$changed" >&2 + exit 1 +fi diff --git a/AGENTS.md b/AGENTS.md index 6ab0e23ca..73fe94ba8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,10 +63,14 @@ named `changelog.d/..md`, where `` is `breaking`, `added`, `changed`, `fixed` or `removed` (see "Changelog fragments" in that skill). Agents must never bypass branch rules (for example with `gh pr merge --admin`). -Before committing, run `make quality`: it applies ruff's fixes, then runs -the checks pr.yml's `quality-guards` job runs, apart from the rules -registry tool's own tests (see "Commit Hygiene" in -`docs/engineering/skills/github-prs.md` for how a local run can differ). +Run `make install` once per clone: it installs the dev environment and turns +on the pre-commit hook, which runs `make quality` before every commit in +every worktree of the clone. In a new worktree, run `make install` again +to create its `.venv`; the hook stops commits until it exists. `make +quality` applies ruff's fixes, then runs the checks pr.yml's +`quality-guards` job runs, apart from the rules registry tool's own tests +(see "Commit Hygiene" in `docs/engineering/skills/github-prs.md` for how +a local run can differ). Don't skip the hook with `--no-verify`. Before creating or sharing a migration PR, run the focused migration guards: @@ -123,7 +127,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### Code quality and types -- Checked: `api.quality.alembic-revisions-unused-imports`, `api.quality.ci-cd-scripts-ruff-exemptions`, `api.quality.command-line-modules-ruff-exemptions`, `api.quality.command-line-scripts-ruff-exemptions`, `api.quality.coverage-measured`, `api.quality.diff-coverage`, `api.quality.environment-variables-load`, `api.quality.file-length`, `api.quality.make-quality`, `api.quality.mypy-v2`, `api.quality.ruff-format`, `api.quality.ruff-lint`, `api.quality.runtime-evaluated-models-resolve`, `api.quality.suppressions-explained`, `api.quality.tests-assert`, `api.quality.tests-pass`, `api.quality.tests-ruff-exemptions`, `api.quality.unittest-ruff-exemptions` +- Checked: `api.quality.alembic-revisions-unused-imports`, `api.quality.ci-cd-scripts-ruff-exemptions`, `api.quality.command-line-modules-ruff-exemptions`, `api.quality.command-line-scripts-ruff-exemptions`, `api.quality.coverage-measured`, `api.quality.diff-coverage`, `api.quality.environment-variables-load`, `api.quality.file-length`, `api.quality.make-quality`, `api.quality.mypy-v2`, `api.quality.pre-commit-hook`, `api.quality.ruff-format`, `api.quality.ruff-lint`, `api.quality.runtime-evaluated-models-resolve`, `api.quality.suppressions-explained`, `api.quality.tests-assert`, `api.quality.tests-pass`, `api.quality.tests-ruff-exemptions`, `api.quality.unittest-ruff-exemptions` ### Observability diff --git a/Makefile b/Makefile index 736532b6d..8b2c316a0 100644 --- a/Makefile +++ b/Makefile @@ -5,7 +5,12 @@ # 3.14 for other uv commands; set UV_PYTHON to test another version. export UV_PYTHON ?= 3.12 +# Turns on the pre-commit hook that runs make quality before each commit +# (.githooks/pre-commit, api.quality.pre-commit-hook), then installs the locked +# dev environment. core.hooksPath is shared by every worktree of the clone; +# outside a git repository there is nothing to turn on. install: + if git rev-parse --git-dir >/dev/null 2>&1; then git config core.hooksPath .githooks; fi uv sync --frozen --extra dev setup-env: diff --git a/README.md b/README.md index 8a03ceb52..0f9cbe66f 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,10 @@ make install make setup-env ``` -`make install` runs `uv sync --frozen --extra dev`, which creates `.venv` with +Run `make install` once per clone. It turns on the pre-commit hook in +`.githooks/`, which runs `make quality` before every commit in every worktree +of the clone (`git config core.hooksPath .githooks`), and runs +`uv sync --frozen --extra dev`, which creates `.venv` with exactly the versions in `uv.lock` on Python 3.12, the version the Cloud Run image runs. `make test`, `make test-env-vars`, `make debug-test`, `make debug`, `make debug-asgi`, `make typecheck-v2` and the towncrier step of diff --git a/changelog.d/4206.added.md b/changelog.d/4206.added.md new file mode 100644 index 000000000..9c58b6f85 --- /dev/null +++ b/changelog.d/4206.added.md @@ -0,0 +1 @@ +Added a git pre-commit hook, turned on by `make install`, that runs `make quality` before every commit in every worktree of the clone and stops the commit when a check fails, when ruff's fixes still need staging, or when `uv` or the dev environment is missing. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index f1a71b6a0..188237d7a 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -189,12 +189,24 @@ the reason a code change or ruff setting couldn't remove the finding ## Commit Hygiene AI agents must run the quality checks before committing their own code -changes: +changes. Run `make install` once per clone: it turns on the pre-commit hook +(`.githooks/pre-commit`, through `git config core.hooksPath .githooks`), +which runs them before every commit (`api.quality.pre-commit-hook`): ```bash -make quality +make install # once per clone, and once in each new worktree for its .venv +make quality # what the hook runs; run it yourself to check sooner ``` +The hook runs `make quality` from the root of the working tree. It stops the +commit when a check fails, or when ruff's fixes changed a file, which it +names: review the fixes, stage them and commit again. It also stops the +commit, saying what to install, when `make`, `uv` or the worktree's `.venv` +is missing. `core.hooksPath` is a setting of the clone, so every worktree +runs the hook, from its own checkout. Don't skip it with +`git commit --no-verify`; CI's `Quality checks` job runs the same checks +and blocks the merge anyway. + `make quality` applies ruff's fixes (`ruff check --fix`, then `ruff format`), then runs `make quality-check`: `uv lock --check`, `ruff check`, `ruff format --check`, Import Linter, mypy and the quality guards diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 3325a5a7f..926ae4e5a 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 70 | 7 | 1 | 8 | 86 | +| mechanical | 71 | 7 | 1 | 8 | 87 | | metric | 2 | 0 | 0 | 0 | 2 | | judgment | 0 | 0 | 0 | 1 | 1 | -**Blocking:** 79 of 89 rules. +**Blocking:** 80 of 90 rules. ## Gaps @@ -377,6 +377,7 @@ flowchart LR | `api.quality.file-length` | A tracked Python file has at most 800 physical lines, or at most 1,500 if it is test code (a file under a tests or fixtures directory, or a conftest.py); a longer file is split by responsibility into modules, and keeps its public import path. | metric | enforced | yes | quality guard `file-length` | `pr.yml:quality-guards` | yes | | `api.quality.make-quality` | make quality is the command for the quality checks, locally and in CI: it applies ruff check --fix and ruff format, then runs make quality-check, which runs uv lock --check, ruff check, ruff format --check, Import Linter, mypy and the quality guards without changing files. Run with a stand-in uv, make quality runs each of them and fails whenever one fails, and make -n prints the same commands. pr.yml's quality-guards job runs make quality and then git diff --exit-code HEAD, with neither step skipped, allowed to fail, or run in another shell or directory, no MAKEFLAGS, GNUMAKEFLAGS or MAKEFILES set for them, and no earlier step writing to GITHUB_ENV or GITHUB_PATH; the Makefile uses no syntax the rules registry doesn't follow and nothing that hides a failing command; and no other pr.yml job's commands run ruff, mypy, lint-imports or the quality guards, directly or through a make target. | mechanical | enforced | yes | pytest `tests/unit/test_make_quality.py` | `pr.yml:test` | yes | | `api.quality.mypy-v2` | Shared query models, FastAPI dependencies, v2 routes and services, the gzip negotiation, the user sign-in and account-deletion libraries, the country list and its startup check pass mypy with untyped definitions disallowed. | mechanical | enforced | yes | mypy | `pr.yml:quality-guards` | — | +| `api.quality.pre-commit-hook` | make install turns on a git pre-commit hook, .githooks/pre-commit, through core.hooksPath, which every worktree of the clone shares. The hook runs make quality from the root of the working tree and stops the commit when a check fails, when ruff's fixes changed a file that still needs staging (naming it), or when make, uv or the worktree's dev environment (.venv) is missing. | mechanical | enforced | yes | pytest `tests/unit/test_pre_commit_hook.py` | `pr.yml:test` | yes | | `api.quality.ruff-format` | All Python files are formatted with ruff format. | mechanical | enforced | yes | ruff format | `pr.yml:quality-guards` | — | | `api.quality.ruff-lint` | All Python code passes ruff check with every ruff rule selected (ALL), with no ratchet, except the ignores in pyproject.toml: COM812 and ISC001, which conflict with ruff format, and D203 and D213, which conflict with the kept D211 and D212. | mechanical | enforced | yes | ruff check (ALL) | `pr.yml:quality-guards` | — | | `api.quality.runtime-evaluated-models-resolve` | Every package model is one ruff treats as runtime-evaluated (a base resolves, through the import in its own module, to a name in runtime-evaluated-base-classes, or is a same-module class that does), and every Pydantic model and SQLAlchemy mapper resolves its annotations once the package is imported. | mechanical | enforced | yes | pytest `tests/unit/test_runtime_evaluated_annotations.py` | `pr.yml:test` | **unproven** | @@ -412,6 +413,8 @@ flowchart LR e_pytest_tests_unit_test_make_quality_py["pytest tests/unit/test_make_quality.py"] r_api_quality_mypy_v2("api.quality.mypy-v2 (enforced)") e_mypy["mypy"] + r_api_quality_pre_commit_hook("api.quality.pre-commit-hook (enforced)") + e_pytest_tests_unit_test_pre_commit_hook_py["pytest tests/unit/test_pre_commit_hook.py"] r_api_quality_ruff_format("api.quality.ruff-format (enforced)") e_ruff_format["ruff format"] r_api_quality_ruff_lint("api.quality.ruff-lint (enforced)") @@ -421,8 +424,6 @@ flowchart LR r_api_quality_suppressions_explained("api.quality.suppressions-explained (ratcheted)") e_quality_guard_suppressions["quality guard suppressions"] j_CI_push_yml_quality_guards["CI push.yml:quality-guards"] - r_api_quality_tests_assert("api.quality.tests-assert (ratcheted)") - e_quality_guard_asserting_tests["quality guard asserting-tests"] r_api_quality_alembic_revisions_unused_imports -.-> gap_unenforced r_api_quality_ci_cd_scripts_ruff_exemptions --> e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___ e_ruff_per_file_ignore_of_N999__S310__S603__S607__T201__TID253_for__github___ --> j_CI_pr_yml_quality_guards @@ -442,6 +443,8 @@ flowchart LR e_pytest_tests_unit_test_make_quality_py --> j_CI_pr_yml_test r_api_quality_mypy_v2 --> e_mypy e_mypy --> j_CI_pr_yml_quality_guards + r_api_quality_pre_commit_hook --> e_pytest_tests_unit_test_pre_commit_hook_py + e_pytest_tests_unit_test_pre_commit_hook_py --> j_CI_pr_yml_test r_api_quality_ruff_format --> e_ruff_format e_ruff_format --> j_CI_pr_yml_quality_guards r_api_quality_ruff_lint --> e_ruff_check__ALL_ @@ -451,22 +454,25 @@ flowchart LR r_api_quality_suppressions_explained --> e_quality_guard_suppressions e_quality_guard_suppressions --> j_CI_pr_yml_quality_guards e_quality_guard_suppressions --> j_CI_push_yml_quality_guards - r_api_quality_tests_assert --> e_quality_guard_asserting_tests - e_quality_guard_asserting_tests --> j_CI_pr_yml_quality_guards - e_quality_guard_asserting_tests --> j_CI_push_yml_quality_guards ``` ```mermaid flowchart LR + r_api_quality_tests_assert("api.quality.tests-assert (ratcheted)") + e_quality_guard_asserting_tests["quality guard asserting-tests"] + j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] + j_CI_push_yml_quality_guards["CI push.yml:quality-guards"] r_api_quality_tests_pass("api.quality.tests-pass (enforced)") e_CI_job_test__check_test_shards["CI job test, check-test-shards"] j_CI_pr_yml_test["CI pr.yml:test"] j_CI_pr_yml_check_test_shards["CI pr.yml:check-test-shards"] r_api_quality_tests_ruff_exemptions("api.quality.tests-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_ANN__ARG001__ARG002__ARG005__D1__DTZ001__DTZ011__INP001__N802__PLC0415__PLR2004__S101__S105__S106__S603__S607__S608__SLF001__T201__TID253_for____tests___["ruff per-file ignore of ANN, ARG001, ARG002, ARG005, D1, DTZ001, DTZ011, INP001, N802, PLC0415, PLR2004, S101, S105, S106, S603, S607, S608, SLF001, T201, TID253 for **/tests/**"] - j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] r_api_quality_unittest_ruff_exemptions("api.quality.unittest-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_PT_for_scripts_rules_tests___["ruff per-file ignore of PT for scripts/rules/tests/**"] + r_api_quality_tests_assert --> e_quality_guard_asserting_tests + e_quality_guard_asserting_tests --> j_CI_pr_yml_quality_guards + e_quality_guard_asserting_tests --> j_CI_push_yml_quality_guards r_api_quality_tests_pass --> e_CI_job_test__check_test_shards e_CI_job_test__check_test_shards --> j_CI_pr_yml_test e_CI_job_test__check_test_shards --> j_CI_pr_yml_check_test_shards diff --git a/rules/registry.toml b/rules/registry.toml index f47a1d642..d2de4cad8 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -1195,6 +1195,39 @@ test = "tests/unit/services/test_microdata_weight_boundaries.py::test_rule_proof # --- Code quality and types ------------------------------------------------ +[[rule]] +id = "api.quality.pre-commit-hook" +category = "quality" +statement = "make install turns on a git pre-commit hook, .githooks/pre-commit, through core.hooksPath, which every worktree of the clone shares. The hook runs make quality from the root of the working tree and stops the commit when a check fails, when ruff's fixes changed a file that still needs staging (naming it), or when make, uv or the worktree's dev environment (.venv) is missing." +rationale = "docs/engineering/skills/github-prs.md#commit-hygiene" +scope = [".githooks/pre-commit", "Makefile"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "pytest" +nodes = [ + "tests/unit/test_pre_commit_hook.py::test_the_repository_s_hook_stops_what_it_should", + "tests/unit/test_pre_commit_hook.py::test_a_commit_runs_make_quality", + "tests/unit/test_pre_commit_hook.py::test_a_failing_check_stops_the_commit", + "tests/unit/test_pre_commit_hook.py::test_fixes_that_need_staging_stop_the_commit_and_are_named", + "tests/unit/test_pre_commit_hook.py::test_unstaged_changes_make_quality_leaves_alone_dont_stop_the_commit", + "tests/unit/test_pre_commit_hook.py::test_a_missing_uv_stops_the_commit_with_a_message", + "tests/unit/test_pre_commit_hook.py::test_a_missing_dev_environment_stops_the_commit_with_a_message", + "tests/unit/test_pre_commit_hook.py::test_every_worktree_of_the_clone_runs_the_hook", + "tests/unit/test_pre_commit_hook.py::test_make_install_turns_the_hook_on", + "tests/unit/test_pre_commit_hook.py::test_make_install_outside_a_git_repository_still_installs", + "tests/unit/test_pre_commit_hook.py::test_the_hook_is_executable_in_git", +] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "make test" +[rule.proof] +bad_fixture = "rules/fixtures/api.quality.pre-commit-hook/rejected" +good_fixture = "rules/fixtures/api.quality.pre-commit-hook/accepted" +test = "tests/unit/test_pre_commit_hook.py::test_rule_proof_pre_commit_hook" + [[rule]] id = "api.quality.make-quality" category = "quality" From f252e780637c075801f465b8f880cd58d3273245 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:43:04 +0400 Subject: [PATCH 3/8] Test that the hook checks what is committed, merges, and safe installs (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 --- .../accepted/pre-commit | 10 +- .../rejected/pre-commit | 4 +- tests/unit/test_pre_commit_hook.py | 405 ++++++++++++++---- 3 files changed, 328 insertions(+), 91 deletions(-) diff --git a/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit b/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit index f674b0ae6..c408523a9 100755 --- a/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit +++ b/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit @@ -1,7 +1,9 @@ #!/bin/sh # Proof fixture for api.quality.pre-commit-hook (accepted): the least a hook -# must do. It runs make quality, fails when make quality fails, and fails when -# make quality changed a file, whose fix still needs staging. -before=$(git diff) +# must do. It stops the commit when uv is missing, when the working tree holds +# changes the commit doesn't (make quality would check them instead), when +# make quality fails, and when make quality changed a file. +command -v uv >/dev/null 2>&1 || exit 1 +git diff --quiet || exit 1 make quality || exit 1 -[ "$(git diff)" = "$before" ] || exit 1 +git diff --quiet || exit 1 diff --git a/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit b/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit index 597cbb00a..659ffe58b 100755 --- a/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit +++ b/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit @@ -1,6 +1,6 @@ #!/bin/sh # Proof fixture for api.quality.pre-commit-hook (rejected): it runs make -# quality but lets the commit through whether the checks fail or ruff's fixes -# changed a file. +# quality but lets the commit through whether uv is missing, the working tree +# holds unstaged changes, the checks fail, or ruff's fixes changed a file. make quality exit 0 diff --git a/tests/unit/test_pre_commit_hook.py b/tests/unit/test_pre_commit_hook.py index 4fcd9310a..15c41aa0c 100644 --- a/tests/unit/test_pre_commit_hook.py +++ b/tests/unit/test_pre_commit_hook.py @@ -1,12 +1,18 @@ """A git pre-commit hook runs make quality (api.quality.pre-commit-hook). -make install turns on .githooks/pre-commit with git config core.hooksPath -.githooks, which every worktree of the clone shares. The hook runs make -quality from the root of the working tree. It fails the commit when a check -fails, when ruff's fixes changed files that still need staging, or when make, -uv or the worktree's dev environment (.venv) is missing. These tests run it -in temporary repositories whose Makefile's quality target stands in for the -real checks. +make install turns on the hooks in .githooks with git config core.hooksPath +.githooks, a setting of the clone that every worktree shares and resolves +against its own checkout. pre-commit runs make quality from the root of the +working tree, and pre-merge-commit runs pre-commit for a merge that commits +without conflicts. make quality checks the working tree, so the hook first +requires it to hold exactly what is committed: no unstaged changes to tracked +files and no untracked files outside .gitignore. The commit stops, saying +why, when that isn't so, when a check fails, when ruff's fixes changed a +file, or when make, uv or the worktree's dev environment is missing. + +These tests run the hooks in temporary repositories whose Makefile's quality +target stands in for the real checks, with a PATH of only the tools they +need, a stand-in uv, and no global or system git settings. """ from __future__ import annotations @@ -16,42 +22,51 @@ import subprocess from pathlib import Path -import pytest - REPO = Path(__file__).resolve().parents[2] -HOOK = REPO / ".githooks" / "pre-commit" +HOOKS = REPO / ".githooks" FIXTURES = REPO / "rules" / "fixtures" / "api.quality.pre-commit-hook" -# Variables of make's own, and git settings outside the test repository, that -# the test's git and make mustn't inherit. -LEAKING_VARIABLES = frozenset( - {"MAKEFLAGS", "MFLAGS", "GNUMAKEFLAGS", "MAKEFILES", "MAKELEVEL", "GIT_DIR"} +# The tools the hooks, make install and these tests run, found through PATH. +TOOLS = ("git", "make", "sh", "find", "dirname", "env") +# A stand-in for uv that records each call when UV_STUB_LOG is set. +STUB_UV = ( + '#!/bin/sh\nif [ -n "${UV_STUB_LOG:-}" ]; then\n' + ' printf \'%s\\n\' "$*" >> "$UV_STUB_LOG"\nfi\nexit 0\n' ) -STUB_UV = "#!/bin/sh\nexit 0\n" PASSING_QUALITY = 'quality:\n\t@echo checked >> "$$QUALITY_MARKER"\n' FAILING_QUALITY = "quality:\n\t@echo 'mypy: 1 error'\n\t@exit 1\n" FIXING_QUALITY = "quality:\n\t@printf 'VALUE = 1\\n' > module.py\n" +# Fixes module.py, as ruff check --fix does, then fails, as mypy can after it. +FIXING_THEN_FAILING_QUALITY = ( + "quality:\n\t@printf 'VALUE = 1\\n' > module.py\n\t@echo 'mypy: 1 error'\n" + "\t@exit 1\n" +) -def _environment(tmp_path: Path, *, with_uv: bool = True) -> dict[str, str]: - """Return an environment with a stand-in uv and no global git settings.""" - stubs = tmp_path / "stubs" - stubs.mkdir(exist_ok=True) - if with_uv: - (stubs / "uv").write_text(STUB_UV) - (stubs / "uv").chmod(0o755) - search_path = [str(stubs)] - for tool in ("git", "make", "sh"): - directory = str(Path(shutil.which(tool) or "/usr/bin").parent) - if directory not in search_path: - search_path.append(directory) +def _environment(tmp_path: Path, *, missing: tuple[str, ...] = ()) -> dict[str, str]: + """Return an environment whose PATH has only TOOLS and a stand-in uv. + + Tools named in ``missing`` are left out. No git or make settings from the + run that started the test reach it. + """ + tools = tmp_path / "tools" + tools.mkdir(exist_ok=True) + for tool in TOOLS: + found = shutil.which(tool) + if tool not in missing and found and not (tools / tool).exists(): + (tools / tool).symlink_to(found) + if "uv" not in missing: + (tools / "uv").write_text(STUB_UV) + (tools / "uv").chmod(0o755) environment = { name: value for name, value in os.environ.items() - if name not in LEAKING_VARIABLES and not name.startswith("GIT_") + if not name.startswith(("GIT_", "UV_")) + and name not in {"MAKEFLAGS", "MFLAGS", "GNUMAKEFLAGS", "MAKEFILES"} + and name != "MAKELEVEL" } environment.update( { - "PATH": os.pathsep.join([*search_path, "/usr/bin", "/bin"]), + "PATH": str(tools), "HOME": str(tmp_path), "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_NOSYSTEM": "1", @@ -78,19 +93,38 @@ def _git( ) +def _make( + cwd: Path, environment: dict[str, str], target: str +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["make", target], + cwd=cwd, + env=environment, + capture_output=True, + text=True, + check=False, + ) + + def _repository( tmp_path: Path, environment: dict[str, str], quality_recipe: str, - hook: Path = HOOK, + hook: Path | None = None, *, dev_environment: bool = True, ) -> Path: - """Create a repository whose commits run ``hook``, with one commit made.""" + """Create a repository with one commit whose commits run the hooks. + + ``hook`` replaces .githooks/pre-commit with another file, such as a proof + fixture. + """ repository = tmp_path / "repository" - (repository / ".githooks").mkdir(parents=True) - shutil.copy(hook, repository / ".githooks" / "pre-commit") - (repository / ".githooks" / "pre-commit").chmod(0o755) + repository.mkdir(parents=True) + shutil.copytree(HOOKS, repository / ".githooks") + if hook is not None: + shutil.copy(hook, repository / ".githooks" / "pre-commit") + (repository / ".githooks" / "pre-commit").chmod(0o755) (repository / "Makefile").write_text(quality_recipe) (repository / "module.py").write_text("VALUE = 0\n") (repository / "other.py").write_text("OTHER = 0\n") @@ -117,18 +151,35 @@ def _commits(repository: Path, environment: dict[str, str]) -> int: return int(counted.stdout) +def _ran(tmp_path: Path) -> int: + """Return how many times the stand-in quality target ran.""" + marker = tmp_path / "quality-ran" + return len(marker.read_text().splitlines()) if marker.exists() else 0 + + def hook_problems(hook: Path, tmp_path: Path) -> list[str]: """Return how ``hook`` fails to stop a commit api.quality.pre-commit-hook stops.""" problems = [] - for name, recipe, blocks in ( - ("a failing check", FAILING_QUALITY, True), - ("ruff's fixes to a staged file", FIXING_QUALITY, True), - ("passing checks", PASSING_QUALITY, False), + for name, recipe, prepare, blocks in ( + ("a failing check", FAILING_QUALITY, None, True), + ("ruff's fixes to a staged file", FIXING_QUALITY, None, True), + ( + "an unstaged change make quality would check", + PASSING_QUALITY, + "unstaged", + True, + ), + ("a missing uv", PASSING_QUALITY, "no uv", True), + ("passing checks", PASSING_QUALITY, None, False), ): scenario = tmp_path / name.replace(" ", "-").replace("'", "") scenario.mkdir(parents=True) - environment = _environment(scenario) + environment = _environment( + scenario, missing=("uv",) if prepare == "no uv" else () + ) repository = _repository(scenario, environment, recipe, hook) + if prepare == "unstaged": + (repository / "other.py").write_text("OTHER = 1\n") result = _commit_a_change(repository, environment) if blocks and result.returncode == 0: problems.append(f"the hook lets a commit through after {name}") @@ -144,12 +195,15 @@ def test_rule_proof_pre_commit_hook(tmp_path: Path): assert rejected == [ "the hook lets a commit through after a failing check", "the hook lets a commit through after ruff's fixes to a staged file", + "the hook lets a commit through after an unstaged change make quality " + "would check", + "the hook lets a commit through after a missing uv", ] assert accepted == [] def test_the_repository_s_hook_stops_what_it_should(tmp_path: Path): - assert hook_problems(HOOK, tmp_path) == [] + assert hook_problems(HOOKS / "pre-commit", tmp_path) == [] def test_a_commit_runs_make_quality(tmp_path: Path): @@ -159,7 +213,7 @@ def test_a_commit_runs_make_quality(tmp_path: Path): result = _commit_a_change(repository, environment) assert result.returncode == 0, result.stderr - assert (tmp_path / "quality-ran").read_text() == "checked\n" + assert _ran(tmp_path) == 1 assert _commits(repository, environment) == 2 @@ -186,23 +240,104 @@ def test_fixes_that_need_staging_stop_the_commit_and_are_named(tmp_path: Path): assert _commits(repository, environment) == 1 -def test_unstaged_changes_make_quality_leaves_alone_dont_stop_the_commit( - tmp_path: Path, -): +def test_fixes_from_an_earlier_failed_run_stop_the_next_commit(tmp_path: Path): + # The first commit fails after ruff fixed module.py in the working tree; + # the fix was never staged, so the next commit must not pass without it. + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, FIXING_THEN_FAILING_QUALITY) + first = _commit_a_change(repository, environment) + (repository / "Makefile").write_text(PASSING_QUALITY) + _git(repository, environment, "add", "Makefile") + + second = _git(repository, environment, "commit", "-m", "change") + + assert first.returncode != 0 + assert second.returncode != 0 + assert "module.py" in second.stderr + assert _commits(repository, environment) == 1 + + +def test_unstaged_changes_stop_the_commit_and_are_named(tmp_path: Path): + # make quality would check them, not what is committed. The name is shown + # as it is, not quoted as git quotes names outside ASCII. environment = _environment(tmp_path) repository = _repository(tmp_path, environment, PASSING_QUALITY) - (repository / "other.py").write_text("OTHER = 1\n") + (repository / "données.py").write_text("DATA = 0\n") + _git(repository, environment, "add", "données.py") + (repository / "données.py").write_text("DATA = 1\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "données.py" in result.stderr + assert "git stash push --keep-index" in result.stderr + assert _ran(tmp_path) == 0 + assert _commits(repository, environment) == 1 + + +def test_untracked_files_stop_the_commit_and_are_named(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "scratch_notes.py").write_text("import os\n") result = _commit_a_change(repository, environment) + assert result.returncode != 0 + assert "scratch_notes.py" in result.stderr + assert _ran(tmp_path) == 0 + assert _commits(repository, environment) == 1 + + +def test_commit_all_runs_make_quality(tmp_path: Path): + # git commit -a stages every tracked change, so nothing is left unstaged. + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "module.py").write_text("VALUE = 2\n") + (repository / "other.py").write_text("OTHER = 1\n") + + result = _git(repository, environment, "commit", "-a", "-m", "change") + assert result.returncode == 0, result.stderr - assert _commits(repository, environment) == 2 + assert _ran(tmp_path) == 1 + + +def test_a_merge_without_conflicts_runs_the_hook(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + _git(repository, environment, "checkout", "--quiet", "-b", "feature") + (repository / "other.py").write_text("OTHER = 1\n") + _git(repository, environment, "commit", "--quiet", "--no-verify", "-am", "feature") + _git(repository, environment, "checkout", "--quiet", "master") + (repository / "module.py").write_text("VALUE = 2\n") + _git(repository, environment, "commit", "--quiet", "--no-verify", "-am", "master") + (repository / "Makefile").write_text(FAILING_QUALITY) + _git(repository, environment, "commit", "--quiet", "--no-verify", "-am", "fail") + + refused = _git(repository, environment, "merge", "--no-edit", "feature") + _git(repository, environment, "merge", "--abort") + (repository / "Makefile").write_text(PASSING_QUALITY) + _git(repository, environment, "commit", "--quiet", "--no-verify", "-am", "pass") + merged = _git(repository, environment, "merge", "--no-edit", "feature") + + assert refused.returncode != 0 + assert "make quality failed" in refused.stderr + assert merged.returncode == 0, merged.stderr + assert _ran(tmp_path) == 1 + + +def test_make_flags_from_the_environment_cant_hide_a_failure(tmp_path: Path): + environment = _environment(tmp_path) + environment["MAKEFLAGS"] = "i" + repository = _repository(tmp_path, environment, FAILING_QUALITY) + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert _commits(repository, environment) == 1 def test_a_missing_uv_stops_the_commit_with_a_message(tmp_path: Path): - environment = _environment(tmp_path, with_uv=False) - if shutil.which("uv", path=environment["PATH"]): - pytest.skip("uv is installed in a system directory") + environment = _environment(tmp_path, missing=("uv",)) repository = _repository(tmp_path, environment, PASSING_QUALITY) result = _commit_a_change(repository, environment) @@ -210,7 +345,17 @@ def test_a_missing_uv_stops_the_commit_with_a_message(tmp_path: Path): assert result.returncode != 0 assert "uv isn't installed" in result.stderr assert "make install" in result.stderr - assert not (tmp_path / "quality-ran").exists() + assert _ran(tmp_path) == 0 + + +def test_a_missing_make_stops_the_commit_with_a_message(tmp_path: Path): + environment = _environment(tmp_path, missing=("make",)) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "make isn't installed" in result.stderr def test_a_missing_dev_environment_stops_the_commit_with_a_message(tmp_path: Path): @@ -224,10 +369,26 @@ def test_a_missing_dev_environment_stops_the_commit_with_a_message(tmp_path: Pat assert result.returncode != 0 assert "no dev environment" in result.stderr assert "make install" in result.stderr - assert not (tmp_path / "quality-ran").exists() + assert _ran(tmp_path) == 0 -def test_every_worktree_of_the_clone_runs_the_hook(tmp_path: Path): +def test_the_dev_environment_can_be_where_uv_project_environment_says( + tmp_path: Path, +): + environment = _environment(tmp_path) + repository = _repository( + tmp_path, environment, PASSING_QUALITY, dev_environment=False + ) + (tmp_path / "environments" / "api").mkdir(parents=True) + environment["UV_PROJECT_ENVIRONMENT"] = str(tmp_path / "environments" / "api") + + result = _commit_a_change(repository, environment) + + assert result.returncode == 0, result.stderr + assert _ran(tmp_path) == 1 + + +def test_each_worktree_runs_its_own_checkout_s_hook(tmp_path: Path): environment = _environment(tmp_path) repository = _repository(tmp_path, environment, PASSING_QUALITY) worktree = tmp_path / "worktree" @@ -242,31 +403,63 @@ def test_every_worktree_of_the_clone_runs_the_hook(tmp_path: Path): str(worktree), ) (worktree / ".venv").mkdir() + # The worktree's branch has a hook that stops every commit. + (worktree / ".githooks" / "pre-commit").write_text( + '#!/bin/sh\necho "the worktree\'s own hook ran" >&2\nexit 1\n' + ) + _git(worktree, environment, "commit", "--quiet", "--no-verify", "-am", "hook") (worktree / "module.py").write_text("VALUE = 3\n") _git(worktree, environment, "add", "module.py") - result = _git(worktree, environment, "commit", "-m", "change in a worktree") + in_worktree = _git(worktree, environment, "commit", "-m", "in the worktree") + in_clone = _commit_a_change(repository, environment) - assert result.returncode == 0, result.stderr - assert (tmp_path / "quality-ran").read_text() == "checked\n" + assert in_worktree.returncode != 0 + assert "the worktree's own hook ran" in in_worktree.stderr + assert in_clone.returncode == 0, in_clone.stderr + assert _ran(tmp_path) == 1 + + +def _source_tree(directory: Path) -> Path: + """Copy the files make install needs into ``directory``.""" + directory.mkdir(parents=True) + shutil.copy(REPO / "Makefile", directory / "Makefile") + shutil.copytree(HOOKS, directory / ".githooks") + return directory def test_make_install_turns_the_hook_on(tmp_path: Path): environment = _environment(tmp_path) - repository = tmp_path / "repository" - repository.mkdir() - shutil.copy(REPO / "Makefile", repository / "Makefile") + repository = _source_tree(tmp_path / "repository") _git(repository, environment, "init", "--quiet") - installed = subprocess.run( - ["make", "install"], - cwd=repository, - env=environment, - capture_output=True, - text=True, - check=False, + installed = _make(repository, environment, "install") + + assert installed.returncode == 0, installed.stderr + hooks_path = _git(repository, environment, "config", "core.hooksPath") + assert hooks_path.stdout.strip() == ".githooks" + + +def test_make_install_in_a_linked_worktree_turns_the_hook_on(tmp_path: Path): + environment = _environment(tmp_path) + repository = _source_tree(tmp_path / "repository") + _git(repository, environment, "init", "--quiet", "--initial-branch=master") + _git(repository, environment, "add", ".") + _git(repository, environment, "commit", "--quiet", "--no-verify", "-m", "start") + worktree = tmp_path / "worktree" + _git( + repository, + environment, + "worktree", + "add", + "--quiet", + "-b", + "other", + str(worktree), ) + installed = _make(worktree, environment, "install") + assert installed.returncode == 0, installed.stderr hooks_path = _git(repository, environment, "config", "core.hooksPath") assert hooks_path.stdout.strip() == ".githooks" @@ -275,30 +468,72 @@ def test_make_install_turns_the_hook_on(tmp_path: Path): def test_make_install_outside_a_git_repository_still_installs(tmp_path: Path): # A Docker build or a source archive has no repository to configure. environment = _environment(tmp_path) - directory = tmp_path / "source" - directory.mkdir() - shutil.copy(REPO / "Makefile", directory / "Makefile") environment["GIT_CEILING_DIRECTORIES"] = str(tmp_path) + environment["UV_STUB_LOG"] = str(tmp_path / "uv-calls") + directory = _source_tree(tmp_path / "source") - installed = subprocess.run( - ["make", "install"], - cwd=directory, - env=environment, - capture_output=True, - text=True, - check=False, - ) + installed = _make(directory, environment, "install") assert installed.returncode == 0, installed.stderr + assert (tmp_path / "uv-calls").read_text() == "sync --frozen --extra dev\n" -def test_the_hook_is_executable_in_git(): - listed = subprocess.run( - ["git", "ls-files", "--stage", "--", ".githooks/pre-commit"], - cwd=REPO, - capture_output=True, - text=True, - check=True, - ).stdout +def test_make_install_leaves_an_enclosing_repository_alone(tmp_path: Path): + # A source archive unpacked inside another repository: that repository's + # settings and hooks aren't make install's to change. + environment = _environment(tmp_path) + environment["UV_STUB_LOG"] = str(tmp_path / "uv-calls") + enclosing = tmp_path / "enclosing" + enclosing.mkdir() + _git(enclosing, environment, "init", "--quiet") + directory = _source_tree(enclosing / "vendor" / "policyengine-api") + + installed = _make(directory, environment, "install") + + assert installed.returncode == 0, installed.stderr + assert "isn't the root of a git working tree" in installed.stderr + hooks_path = _git(enclosing, environment, "config", "core.hooksPath") + assert hooks_path.stdout == "" + assert (tmp_path / "uv-calls").read_text() == "sync --frozen --extra dev\n" + + +def test_make_install_keeps_another_hooks_path(tmp_path: Path): + environment = _environment(tmp_path) + repository = _source_tree(tmp_path / "repository") + _git(repository, environment, "init", "--quiet") + _git(repository, environment, "config", "core.hooksPath", "/team/hooks") + + installed = _make(repository, environment, "install") + + assert installed.returncode == 0, installed.stderr + assert "core.hooksPath is already /team/hooks" in installed.stderr + hooks_path = _git(repository, environment, "config", "core.hooksPath") + assert hooks_path.stdout.strip() == "/team/hooks" + + +def test_make_install_keeps_the_clone_s_own_hooks_running(tmp_path: Path): + # core.hooksPath would stop git running the hooks in .git/hooks. + environment = _environment(tmp_path) + repository = _source_tree(tmp_path / "repository") + _git(repository, environment, "init", "--quiet") + own_hook = repository / ".git" / "hooks" / "pre-push" + own_hook.write_text("#!/bin/sh\nexit 0\n") + own_hook.chmod(0o755) + + installed = _make(repository, environment, "install") + + assert installed.returncode == 0, installed.stderr + assert "pre-push" in installed.stderr + hooks_path = _git(repository, environment, "config", "core.hooksPath") + assert hooks_path.stdout == "" + + +def test_the_hooks_are_executable_in_git(tmp_path: Path): + environment = _environment(tmp_path) + listed = _git( + REPO, environment, "ls-files", "--stage", "--", ".githooks/" + ).stdout.splitlines() + modes = {entry.split("\t")[1]: entry.split()[0] for entry in listed} - assert listed.startswith("100755 ") + assert modes[".githooks/pre-commit"] == "100755" + assert modes[".githooks/pre-merge-commit"] == "100755" From c764aa1c7d50986410855d4eec56d3e5d659ab6e Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:45:13 +0400 Subject: [PATCH 4/8] Check what is committed, run on merges, and turn the hooks on safely - .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 --- .githooks/pre-commit | 64 +++++++++++++-------------- .githooks/pre-merge-commit | 4 ++ .githooks/turn-on.sh | 37 ++++++++++++++++ AGENTS.md | 17 ++++--- Makefile | 11 ++--- README.md | 6 +-- changelog.d/4206.added.md | 2 +- docs/engineering/skills/github-prs.md | 41 ++++++++++++----- docs/engineering/skills/testing.md | 3 +- docs/generated/rules.md | 2 +- rules/registry.toml | 19 ++++++-- 11 files changed, 141 insertions(+), 65 deletions(-) create mode 100755 .githooks/pre-merge-commit create mode 100755 .githooks/turn-on.sh diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 4acf3e011..bd070e3db 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -1,14 +1,18 @@ #!/bin/sh # The repository's pre-commit hook (api.quality.pre-commit-hook). It runs -# make quality before each commit, so the checks pull-request CI runs fail -# here first. make install turns it on with git config core.hooksPath -# .githooks; every worktree of the clone shares that setting and runs its own -# checkout's copy of this file. git commit --no-verify skips it, and CI's -# Quality checks job still runs the same checks. +# make quality before git commit, and pre-merge-commit runs it before a merge +# commits without conflicts, so the checks pull-request CI runs fail here +# first. make install turns both on with git config core.hooksPath .githooks; +# every worktree of the clone shares that setting and runs its own checkout's +# copy. git commit --no-verify skips them, git's cherry-pick, revert and +# rebase make commits without them, and CI's Quality checks job still runs +# the same checks. # -# The commit stops when make quality fails, when ruff's fixes changed a file -# that still needs staging, or when make, uv or the worktree's .venv is -# missing. +# make quality checks the working tree, so the hook first requires it to hold +# exactly what is being committed: no unstaged changes to tracked files and no +# untracked files outside .gitignore. The commit stops, saying why, when that +# isn't so, when make quality fails, when ruff's fixes changed a file, or when +# make, uv or the worktree's dev environment is missing. set -u fail() { @@ -20,36 +24,32 @@ root=$(git rev-parse --show-toplevel) || fail "can't find the root of this working tree." cd "$root" || fail "can't enter $root." +# make's own variables from the environment could make it ignore a failing +# check or read another Makefile. +unset MAKEFLAGS MFLAGS GNUMAKEFLAGS MAKEFILES MAKELEVEL + command -v make >/dev/null 2>&1 || fail "make isn't installed, so make quality can't run. Install make, then commit again." command -v uv >/dev/null 2>&1 || fail "uv isn't installed, so make quality can't run. Install uv (https://docs.astral.sh/uv/), run make install, then commit again." -[ -d .venv ] || - fail "this worktree has no dev environment (.venv). Run make install here, then commit again." - -# Each tracked file with changes not staged for this commit, with a hash of -# its content. A line that differs after make quality is a file it changed. -unstaged_files() { - git diff --name-only | while IFS= read -r path; do - if [ -f "$path" ]; then - printf '%s %s\n' "$(git hash-object -- "$path")" "$path" - else - printf 'deleted %s\n' "$path" - fi - done -} +environment=${UV_PROJECT_ENVIRONMENT:-.venv} +[ -d "$environment" ] || + fail "this worktree has no dev environment ($environment). Run make install here, then commit again." -before=$(mktemp) || fail "can't create a temporary file." -after=$(mktemp) || fail "can't create a temporary file." -trap 'rm -f "$before" "$after"' EXIT +# Names as they are, not quoted as git quotes names outside ASCII. +unstaged=$(git -c core.quotePath=false diff --name-only) +[ -z "$unstaged" ] || + fail "make quality checks the working tree, which holds changes this commit doesn't. Stage them, or set them aside with git stash push --keep-index, then commit again: +$unstaged" +untracked=$(git -c core.quotePath=false ls-files --others --exclude-standard) +[ -z "$untracked" ] || + fail "make quality checks the working tree, which holds files git doesn't track. Add them, remove them, or list them in .git/info/exclude, then commit again: +$untracked" -unstaged_files >"$before" make quality || fail "make quality failed. Fix what it reported above, then commit again." -unstaged_files >"$after" -changed=$(grep -Fxv -f "$before" "$after" | sed 's/^[^ ]* //') -if [ -n "$changed" ]; then - printf 'pre-commit: make quality changed these files. Review the fixes, stage them, then commit again:\n%s\n' "$changed" >&2 - exit 1 -fi +fixed=$(git -c core.quotePath=false diff --name-only) +[ -z "$fixed" ] || + fail "make quality changed these files. Review the fixes, stage them, then commit again: +$fixed" diff --git a/.githooks/pre-merge-commit b/.githooks/pre-merge-commit new file mode 100755 index 000000000..282555846 --- /dev/null +++ b/.githooks/pre-merge-commit @@ -0,0 +1,4 @@ +#!/bin/sh +# git runs this hook, not pre-commit, before a merge that commits without +# conflicts (api.quality.pre-commit-hook). It runs the same checks. +exec "$(dirname "$0")/pre-commit" diff --git a/.githooks/turn-on.sh b/.githooks/turn-on.sh new file mode 100755 index 000000000..e8793b4ac --- /dev/null +++ b/.githooks/turn-on.sh @@ -0,0 +1,37 @@ +#!/bin/sh +# make install runs this to turn on the hooks in .githooks for the clone, with +# git config core.hooksPath .githooks (api.quality.pre-commit-hook). It +# changes nothing, and says why, when this directory isn't the root of a git +# working tree, when core.hooksPath already names another directory, or when +# the clone has hooks of its own that core.hooksPath would stop git running. +# It never fails, so make install still installs. +set -u + +here=$(pwd -P) +# Outside a git repository, such as a source archive, there's nothing to do. +root=$(git rev-parse --show-toplevel 2>/dev/null) || exit 0 +if [ "$(cd "$root" && pwd -P)" != "$here" ]; then + echo "make install: $here isn't the root of a git working tree ($root is), so the pre-commit hook isn't turned on." >&2 + exit 0 +fi + +current=$(git config --get core.hooksPath) +if [ "$current" = ".githooks" ]; then + exit 0 +fi +if [ -n "$current" ]; then + echo "make install: core.hooksPath is already $current, so the pre-commit hook isn't turned on. Run git config core.hooksPath .githooks to use it instead, or call .githooks/pre-commit from your own hook." >&2 + exit 0 +fi + +hooks=$(git rev-parse --git-path hooks) +own=$(find "$hooks" -type f ! -name '*.sample' 2>/dev/null) +if [ -n "$own" ]; then + echo "make install: the pre-commit hook isn't turned on, because git would then stop running these hooks of the clone's own: +$own +Move them into .githooks, or call .githooks/pre-commit from them, then run git config core.hooksPath .githooks." >&2 + exit 0 +fi + +git config core.hooksPath .githooks && + echo "make install: turned on the pre-commit hook for every worktree of this clone (core.hooksPath = .githooks)." diff --git a/AGENTS.md b/AGENTS.md index 73fe94ba8..50694e54c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,13 +64,16 @@ named `changelog.d/..md`, where `` is `breaking`, `added`, Agents must never bypass branch rules (for example with `gh pr merge --admin`). Run `make install` once per clone: it installs the dev environment and turns -on the pre-commit hook, which runs `make quality` before every commit in -every worktree of the clone. In a new worktree, run `make install` again -to create its `.venv`; the hook stops commits until it exists. `make -quality` applies ruff's fixes, then runs the checks pr.yml's -`quality-guards` job runs, apart from the rules registry tool's own tests -(see "Commit Hygiene" in `docs/engineering/skills/github-prs.md` for how -a local run can differ). Don't skip the hook with `--no-verify`. +on the git hooks in `.githooks`, which run `make quality` before each +`git commit` and each merge commit, in every worktree of the clone. In a new +worktree, run `make install` again to create its `.venv`; the hook stops +commits until it exists. Stage everything the commit needs first: the hook +stops a commit while the working tree holds unstaged changes or untracked +files, because `make quality` would check them instead of what is +committed. `make quality` applies ruff's fixes, then runs the checks +pr.yml's `quality-guards` job runs, apart from the rules registry tool's +own tests (see "Commit Hygiene" in `docs/engineering/skills/github-prs.md`). +Don't skip the hook with `--no-verify`. Before creating or sharing a migration PR, run the focused migration guards: diff --git a/Makefile b/Makefile index 8b2c316a0..7fcbde12a 100644 --- a/Makefile +++ b/Makefile @@ -5,12 +5,13 @@ # 3.14 for other uv commands; set UV_PYTHON to test another version. export UV_PYTHON ?= 3.12 -# Turns on the pre-commit hook that runs make quality before each commit -# (.githooks/pre-commit, api.quality.pre-commit-hook), then installs the locked -# dev environment. core.hooksPath is shared by every worktree of the clone; -# outside a git repository there is nothing to turn on. +# Turns on the hooks in .githooks, which run make quality before each commit +# (api.quality.pre-commit-hook), then installs the locked dev environment. +# core.hooksPath is shared by every worktree of the clone. .githooks/turn-on.sh +# says why it changes nothing outside the root of a git working tree, when +# core.hooksPath is already set, or when the clone has hooks of its own. install: - if git rev-parse --git-dir >/dev/null 2>&1; then git config core.hooksPath .githooks; fi + sh .githooks/turn-on.sh uv sync --frozen --extra dev setup-env: diff --git a/README.md b/README.md index 0f9cbe66f..7a95fde2f 100644 --- a/README.md +++ b/README.md @@ -39,9 +39,9 @@ make install make setup-env ``` -Run `make install` once per clone. It turns on the pre-commit hook in -`.githooks/`, which runs `make quality` before every commit in every worktree -of the clone (`git config core.hooksPath .githooks`), and runs +Run `make install` once per clone. It turns on the git hooks in `.githooks/`, +which run `make quality` before each `git commit` and each merge commit in +every worktree of the clone (`git config core.hooksPath .githooks`), and runs `uv sync --frozen --extra dev`, which creates `.venv` with exactly the versions in `uv.lock` on Python 3.12, the version the Cloud Run image runs. `make test`, `make test-env-vars`, `make debug-test`, `make debug`, diff --git a/changelog.d/4206.added.md b/changelog.d/4206.added.md index 9c58b6f85..92792433d 100644 --- a/changelog.d/4206.added.md +++ b/changelog.d/4206.added.md @@ -1 +1 @@ -Added a git pre-commit hook, turned on by `make install`, that runs `make quality` before every commit in every worktree of the clone and stops the commit when a check fails, when ruff's fixes still need staging, or when `uv` or the dev environment is missing. +Added git hooks, turned on by `make install`, that run `make quality` before each `git commit` and each merge commit in every worktree of the clone, and stop the commit when the working tree holds changes the commit doesn't, when a check fails, when ruff's fixes still need staging, or when `uv` or the dev environment is missing. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 188237d7a..74348841b 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -189,23 +189,42 @@ the reason a code change or ruff setting couldn't remove the finding ## Commit Hygiene AI agents must run the quality checks before committing their own code -changes. Run `make install` once per clone: it turns on the pre-commit hook -(`.githooks/pre-commit`, through `git config core.hooksPath .githooks`), -which runs them before every commit (`api.quality.pre-commit-hook`): +changes. Run `make install` once per clone: it turns on the git hooks in +`.githooks` (through `git config core.hooksPath .githooks`), which run them +before each `git commit` and each merge commit +(`api.quality.pre-commit-hook`): ```bash make install # once per clone, and once in each new worktree for its .venv make quality # what the hook runs; run it yourself to check sooner ``` -The hook runs `make quality` from the root of the working tree. It stops the -commit when a check fails, or when ruff's fixes changed a file, which it -names: review the fixes, stage them and commit again. It also stops the -commit, saying what to install, when `make`, `uv` or the worktree's `.venv` -is missing. `core.hooksPath` is a setting of the clone, so every worktree -runs the hook, from its own checkout. Don't skip it with -`git commit --no-verify`; CI's `Quality checks` job runs the same checks -and blocks the merge anyway. +`.githooks/pre-commit` runs `make quality` from the root of the working +tree, and `.githooks/pre-merge-commit` runs it for a merge that commits +without conflicts. `make quality` checks the working tree, not the commit, +so the hook first requires the two to match. It stops the commit, naming +the files and saying what to do: + +- when tracked files have unstaged changes: stage them, or set them aside + with `git stash push --keep-index`. This covers fixes `make quality` made + in an earlier run, and `git commit ` while other files are + changed; +- when there are untracked files outside `.gitignore`: add them, remove + them, or list them in `.git/info/exclude`; +- when a check fails; +- when ruff's fixes changed a file: review the fixes, stage them and + commit again; +- when `make`, `uv` or the worktree's dev environment (`.venv`, or + `UV_PROJECT_ENVIRONMENT`) is missing. + +`core.hooksPath` is a setting of the clone, so every worktree runs the +hooks, each from its own checkout; a worktree on a branch from before the +hooks runs none. `make install` leaves `core.hooksPath` alone, and says +why, when it would change an enclosing repository, replace another +`core.hooksPath`, or stop the clone's own hooks in `.git/hooks`. git's +cherry-pick, revert and rebase make commits without hooks. Don't skip them +with `git commit --no-verify`; CI's `Quality checks` job runs the same +checks and blocks the merge anyway. `make quality` applies ruff's fixes (`ruff check --fix`, then `ruff format`), then runs `make quality-check`: `uv lock --check`, `ruff check`, diff --git a/docs/engineering/skills/testing.md b/docs/engineering/skills/testing.md index 2bb1a9be0..c7f75be52 100644 --- a/docs/engineering/skills/testing.md +++ b/docs/engineering/skills/testing.md @@ -7,7 +7,8 @@ Use this skill whenever adding, moving, or reviewing tests. Tests run in the environment `uv.lock` defines, on Python 3.12, the same versions the Cloud Run image installs (`gcp/cloud_run/Dockerfile`): -- `make install` runs `uv sync --frozen --extra dev`, and `make test` runs +- `make install` turns on the git hooks in `.githooks` and runs + `uv sync --frozen --extra dev`, and `make test` runs pytest and coverage through `uv run --frozen --extra dev`. The Makefile sets `UV_PYTHON=3.12` unless it is already set, because `.python-version` selects 3.14. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 926ae4e5a..cc5db7488 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -377,7 +377,7 @@ flowchart LR | `api.quality.file-length` | A tracked Python file has at most 800 physical lines, or at most 1,500 if it is test code (a file under a tests or fixtures directory, or a conftest.py); a longer file is split by responsibility into modules, and keeps its public import path. | metric | enforced | yes | quality guard `file-length` | `pr.yml:quality-guards` | yes | | `api.quality.make-quality` | make quality is the command for the quality checks, locally and in CI: it applies ruff check --fix and ruff format, then runs make quality-check, which runs uv lock --check, ruff check, ruff format --check, Import Linter, mypy and the quality guards without changing files. Run with a stand-in uv, make quality runs each of them and fails whenever one fails, and make -n prints the same commands. pr.yml's quality-guards job runs make quality and then git diff --exit-code HEAD, with neither step skipped, allowed to fail, or run in another shell or directory, no MAKEFLAGS, GNUMAKEFLAGS or MAKEFILES set for them, and no earlier step writing to GITHUB_ENV or GITHUB_PATH; the Makefile uses no syntax the rules registry doesn't follow and nothing that hides a failing command; and no other pr.yml job's commands run ruff, mypy, lint-imports or the quality guards, directly or through a make target. | mechanical | enforced | yes | pytest `tests/unit/test_make_quality.py` | `pr.yml:test` | yes | | `api.quality.mypy-v2` | Shared query models, FastAPI dependencies, v2 routes and services, the gzip negotiation, the user sign-in and account-deletion libraries, the country list and its startup check pass mypy with untyped definitions disallowed. | mechanical | enforced | yes | mypy | `pr.yml:quality-guards` | — | -| `api.quality.pre-commit-hook` | make install turns on a git pre-commit hook, .githooks/pre-commit, through core.hooksPath, which every worktree of the clone shares. The hook runs make quality from the root of the working tree and stops the commit when a check fails, when ruff's fixes changed a file that still needs staging (naming it), or when make, uv or the worktree's dev environment (.venv) is missing. | mechanical | enforced | yes | pytest `tests/unit/test_pre_commit_hook.py` | `pr.yml:test` | yes | +| `api.quality.pre-commit-hook` | make install turns on the git hooks in .githooks through core.hooksPath, a setting of the clone that every worktree shares and resolves against its own checkout, unless that would change an enclosing repository, replace another core.hooksPath, or stop the clone's own hooks, which it says. pre-commit runs make quality from the root of the working tree before git commit, and pre-merge-commit runs it before a merge commits without conflicts. Because make quality checks the working tree, the hook stops the commit when the working tree holds unstaged changes to tracked files or untracked files outside .gitignore, when make quality fails, when ruff's fixes changed a file, or when make, uv or the dev environment is missing, naming the files or what to install. | mechanical | enforced | yes | pytest `tests/unit/test_pre_commit_hook.py` | `pr.yml:test` | yes | | `api.quality.ruff-format` | All Python files are formatted with ruff format. | mechanical | enforced | yes | ruff format | `pr.yml:quality-guards` | — | | `api.quality.ruff-lint` | All Python code passes ruff check with every ruff rule selected (ALL), with no ratchet, except the ignores in pyproject.toml: COM812 and ISC001, which conflict with ruff format, and D203 and D213, which conflict with the kept D211 and D212. | mechanical | enforced | yes | ruff check (ALL) | `pr.yml:quality-guards` | — | | `api.quality.runtime-evaluated-models-resolve` | Every package model is one ruff treats as runtime-evaluated (a base resolves, through the import in its own module, to a name in runtime-evaluated-base-classes, or is a same-module class that does), and every Pydantic model and SQLAlchemy mapper resolves its annotations once the package is imported. | mechanical | enforced | yes | pytest `tests/unit/test_runtime_evaluated_annotations.py` | `pr.yml:test` | **unproven** | diff --git a/rules/registry.toml b/rules/registry.toml index d2de4cad8..e5943abb4 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -1198,7 +1198,7 @@ test = "tests/unit/services/test_microdata_weight_boundaries.py::test_rule_proof [[rule]] id = "api.quality.pre-commit-hook" category = "quality" -statement = "make install turns on a git pre-commit hook, .githooks/pre-commit, through core.hooksPath, which every worktree of the clone shares. The hook runs make quality from the root of the working tree and stops the commit when a check fails, when ruff's fixes changed a file that still needs staging (naming it), or when make, uv or the worktree's dev environment (.venv) is missing." +statement = "make install turns on the git hooks in .githooks through core.hooksPath, a setting of the clone that every worktree shares and resolves against its own checkout, unless that would change an enclosing repository, replace another core.hooksPath, or stop the clone's own hooks, which it says. pre-commit runs make quality from the root of the working tree before git commit, and pre-merge-commit runs it before a merge commits without conflicts. Because make quality checks the working tree, the hook stops the commit when the working tree holds unstaged changes to tracked files or untracked files outside .gitignore, when make quality fails, when ruff's fixes changed a file, or when make, uv or the dev environment is missing, naming the files or what to install." rationale = "docs/engineering/skills/github-prs.md#commit-hygiene" scope = [".githooks/pre-commit", "Makefile"] tier = "mechanical" @@ -1211,13 +1211,24 @@ nodes = [ "tests/unit/test_pre_commit_hook.py::test_a_commit_runs_make_quality", "tests/unit/test_pre_commit_hook.py::test_a_failing_check_stops_the_commit", "tests/unit/test_pre_commit_hook.py::test_fixes_that_need_staging_stop_the_commit_and_are_named", - "tests/unit/test_pre_commit_hook.py::test_unstaged_changes_make_quality_leaves_alone_dont_stop_the_commit", + "tests/unit/test_pre_commit_hook.py::test_fixes_from_an_earlier_failed_run_stop_the_next_commit", + "tests/unit/test_pre_commit_hook.py::test_unstaged_changes_stop_the_commit_and_are_named", + "tests/unit/test_pre_commit_hook.py::test_untracked_files_stop_the_commit_and_are_named", + "tests/unit/test_pre_commit_hook.py::test_commit_all_runs_make_quality", + "tests/unit/test_pre_commit_hook.py::test_a_merge_without_conflicts_runs_the_hook", + "tests/unit/test_pre_commit_hook.py::test_make_flags_from_the_environment_cant_hide_a_failure", "tests/unit/test_pre_commit_hook.py::test_a_missing_uv_stops_the_commit_with_a_message", + "tests/unit/test_pre_commit_hook.py::test_a_missing_make_stops_the_commit_with_a_message", "tests/unit/test_pre_commit_hook.py::test_a_missing_dev_environment_stops_the_commit_with_a_message", - "tests/unit/test_pre_commit_hook.py::test_every_worktree_of_the_clone_runs_the_hook", + "tests/unit/test_pre_commit_hook.py::test_the_dev_environment_can_be_where_uv_project_environment_says", + "tests/unit/test_pre_commit_hook.py::test_each_worktree_runs_its_own_checkout_s_hook", "tests/unit/test_pre_commit_hook.py::test_make_install_turns_the_hook_on", + "tests/unit/test_pre_commit_hook.py::test_make_install_in_a_linked_worktree_turns_the_hook_on", "tests/unit/test_pre_commit_hook.py::test_make_install_outside_a_git_repository_still_installs", - "tests/unit/test_pre_commit_hook.py::test_the_hook_is_executable_in_git", + "tests/unit/test_pre_commit_hook.py::test_make_install_leaves_an_enclosing_repository_alone", + "tests/unit/test_pre_commit_hook.py::test_make_install_keeps_another_hooks_path", + "tests/unit/test_pre_commit_hook.py::test_make_install_keeps_the_clone_s_own_hooks_running", + "tests/unit/test_pre_commit_hook.py::test_the_hooks_are_executable_in_git", ] [[rule.ci]] workflow = ".github/workflows/pr.yml" From 26ee1ab909f9e7ea15b47491e130dacbe9c62111 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:09:20 +0400 Subject: [PATCH 5/8] Test hidden Python files, make variables, git's skipped files and safer 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 --- .../accepted/pre-commit | 9 +- .../rejected/pre-commit | 5 +- tests/unit/test_locked_test_environments.py | 16 ++- tests/unit/test_pre_commit_hook.py | 130 +++++++++++++++++- 4 files changed, 151 insertions(+), 9 deletions(-) diff --git a/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit b/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit index c408523a9..941da3a59 100755 --- a/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit +++ b/rules/fixtures/api.quality.pre-commit-hook/accepted/pre-commit @@ -1,9 +1,12 @@ #!/bin/sh # Proof fixture for api.quality.pre-commit-hook (accepted): the least a hook -# must do. It stops the commit when uv is missing, when the working tree holds -# changes the commit doesn't (make quality would check them instead), when -# make quality fails, and when make quality changed a file. +# must do. It stops the commit when uv or the dev environment is missing, when +# the working tree holds unstaged changes or untracked files (make quality +# would check them instead of what is committed), when make quality fails, +# and when make quality changed a file. command -v uv >/dev/null 2>&1 || exit 1 +[ -d .venv ] || exit 1 git diff --quiet || exit 1 +[ -z "$(git ls-files --others --exclude-standard)" ] || exit 1 make quality || exit 1 git diff --quiet || exit 1 diff --git a/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit b/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit index 659ffe58b..3130f3805 100755 --- a/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit +++ b/rules/fixtures/api.quality.pre-commit-hook/rejected/pre-commit @@ -1,6 +1,7 @@ #!/bin/sh # Proof fixture for api.quality.pre-commit-hook (rejected): it runs make -# quality but lets the commit through whether uv is missing, the working tree -# holds unstaged changes, the checks fail, or ruff's fixes changed a file. +# quality but lets the commit through whether uv or the dev environment is +# missing, the working tree holds unstaged changes or untracked files, the +# checks fail, or ruff's fixes changed a file. make quality exit 0 diff --git a/tests/unit/test_locked_test_environments.py b/tests/unit/test_locked_test_environments.py index caad15489..80913def3 100644 --- a/tests/unit/test_locked_test_environments.py +++ b/tests/unit/test_locked_test_environments.py @@ -17,7 +17,11 @@ import pytest -from scripts.rules.locked_installs import executed_lines, makefile_recipes +from scripts.rules.locked_installs import ( + classify_command, + executed_lines, + makefile_recipes, +) from scripts.rules.workflows import parse_jobs REPO = Path(__file__).resolve().parents[2] @@ -113,7 +117,15 @@ def test_detects_every_known_test_job(): def test_make_install_syncs_the_locked_development_environment(): - assert makefile_recipes(REPO)["install"] == [LOCKED_SYNC] + recipe = makefile_recipes(REPO)["install"] + + assert LOCKED_SYNC in recipe + # Its other commands, such as turning on the git hooks, install nothing. + assert [ + command + for command in recipe + if command != LOCKED_SYNC and classify_command(command) is not None + ] == [] @pytest.mark.parametrize("target", ["test", "test-env-vars", "debug-test"]) diff --git a/tests/unit/test_pre_commit_hook.py b/tests/unit/test_pre_commit_hook.py index 15c41aa0c..f0a842a00 100644 --- a/tests/unit/test_pre_commit_hook.py +++ b/tests/unit/test_pre_commit_hook.py @@ -22,11 +22,13 @@ import subprocess from pathlib import Path +import pytest + REPO = Path(__file__).resolve().parents[2] HOOKS = REPO / ".githooks" FIXTURES = REPO / "rules" / "fixtures" / "api.quality.pre-commit-hook" # The tools the hooks, make install and these tests run, found through PATH. -TOOLS = ("git", "make", "sh", "find", "dirname", "env") +TOOLS = ("git", "make", "sh", "find", "env") # A stand-in for uv that records each call when UV_STUB_LOG is set. STUB_UV = ( '#!/bin/sh\nif [ -n "${UV_STUB_LOG:-}" ]; then\n' @@ -35,6 +37,10 @@ PASSING_QUALITY = 'quality:\n\t@echo checked >> "$$QUALITY_MARKER"\n' FAILING_QUALITY = "quality:\n\t@echo 'mypy: 1 error'\n\t@exit 1\n" FIXING_QUALITY = "quality:\n\t@printf 'VALUE = 1\\n' > module.py\n" +# The real Makefile's shape: quality runs quality-check through $(MAKE). +RECURSIVE_FAILING_QUALITY = ( + "quality:\n\t$(MAKE) quality-check\n\nquality-check:\n\t@exit 1\n" +) # Fixes module.py, as ruff check --fix does, then fails, as mypy can after it. FIXING_THEN_FAILING_QUALITY = ( "quality:\n\t@printf 'VALUE = 1\\n' > module.py\n\t@echo 'mypy: 1 error'\n" @@ -68,6 +74,8 @@ def _environment(tmp_path: Path, *, missing: tuple[str, ...] = ()) -> dict[str, { "PATH": str(tools), "HOME": str(tmp_path), + # git reads $XDG_CONFIG_HOME/git/ignore even without a global config. + "XDG_CONFIG_HOME": str(tmp_path / "config"), "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_NOSYSTEM": "1", "GIT_AUTHOR_NAME": "test", @@ -170,6 +178,8 @@ def hook_problems(hook: Path, tmp_path: Path) -> list[str]: True, ), ("a missing uv", PASSING_QUALITY, "no uv", True), + ("an untracked file", PASSING_QUALITY, "untracked", True), + ("a missing dev environment", PASSING_QUALITY, "no venv", True), ("passing checks", PASSING_QUALITY, None, False), ): scenario = tmp_path / name.replace(" ", "-").replace("'", "") @@ -177,7 +187,11 @@ def hook_problems(hook: Path, tmp_path: Path) -> list[str]: environment = _environment( scenario, missing=("uv",) if prepare == "no uv" else () ) - repository = _repository(scenario, environment, recipe, hook) + repository = _repository( + scenario, environment, recipe, hook, dev_environment=prepare != "no venv" + ) + if prepare == "untracked": + (repository / "scratch.py").write_text("SCRATCH = 0\n") if prepare == "unstaged": (repository / "other.py").write_text("OTHER = 1\n") result = _commit_a_change(repository, environment) @@ -198,6 +212,8 @@ def test_rule_proof_pre_commit_hook(tmp_path: Path): "the hook lets a commit through after an unstaged change make quality " "would check", "the hook lets a commit through after a missing uv", + "the hook lets a commit through after an untracked file", + "the hook lets a commit through after a missing dev environment", ] assert accepted == [] @@ -528,6 +544,116 @@ def test_make_install_keeps_the_clone_s_own_hooks_running(tmp_path: Path): assert hooks_path.stdout == "" +def test_python_files_hidden_by_local_excludes_stop_the_commit(tmp_path: Path): + # mypy and Import Linter read every Python file on disk, whatever git + # ignores outside the repository's .gitignore files. + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "helper.py").write_text("HELPER = 0\n") + (repository / ".git" / "info" / "exclude").write_text("helper.py\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "helper.py" in result.stderr + assert _ran(tmp_path) == 0 + + +def test_other_files_hidden_by_local_excludes_dont_stop_the_commit(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "notes.txt").write_text("notes\n") + (repository / ".git" / "info" / "exclude").write_text("notes.txt\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode == 0, result.stderr + assert _ran(tmp_path) == 1 + + +@pytest.mark.parametrize("variable", ["MAKE", "MAKE_COMMAND"]) +def test_make_from_the_environment_cant_skip_the_checks(variable, tmp_path: Path): + # make takes $(MAKE) from the environment, so `true` would skip + # quality-check. + environment = _environment(tmp_path) + # An absolute path: the test's PATH has no true. + environment[variable] = shutil.which("true") or "/usr/bin/true" + repository = _repository(tmp_path, environment, RECURSIVE_FAILING_QUALITY) + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert _commits(repository, environment) == 1 + + +@pytest.mark.parametrize("flag", ["--skip-worktree", "--assume-unchanged"]) +def test_files_git_doesnt_compare_stop_the_commit(flag, tmp_path: Path): + # git diff doesn't compare them with the commit, so make quality could + # check a copy the commit doesn't hold. + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + _git(repository, environment, "update-index", flag, "other.py") + (repository / "other.py").write_text("OTHER = 1\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "other.py" in result.stderr + assert _ran(tmp_path) == 0 + + +def test_make_install_keeps_a_symlinked_hook_running(tmp_path: Path): + environment = _environment(tmp_path) + repository = _source_tree(tmp_path / "repository") + _git(repository, environment, "init", "--quiet") + script = tmp_path / "team-pre-push" + script.write_text("#!/bin/sh\nexit 0\n") + script.chmod(0o755) + (repository / ".git" / "hooks" / "pre-push").symlink_to(script) + + installed = _make(repository, environment, "install") + + assert installed.returncode == 0, installed.stderr + assert "pre-push" in installed.stderr + hooks_path = _git(repository, environment, "config", "core.hooksPath") + assert hooks_path.stdout == "" + + +def test_make_install_keeps_a_symlinked_hooks_directory_running(tmp_path: Path): + environment = _environment(tmp_path) + repository = _source_tree(tmp_path / "repository") + _git(repository, environment, "init", "--quiet") + team_hooks = tmp_path / "team-hooks" + team_hooks.mkdir() + (team_hooks / "pre-push").write_text("#!/bin/sh\nexit 0\n") + (team_hooks / "pre-push").chmod(0o755) + shutil.rmtree(repository / ".git" / "hooks") + (repository / ".git" / "hooks").symlink_to(team_hooks) + + installed = _make(repository, environment, "install") + + assert installed.returncode == 0, installed.stderr + assert "pre-push" in installed.stderr + hooks_path = _git(repository, environment, "config", "core.hooksPath") + assert hooks_path.stdout == "" + + +def test_make_install_still_installs_when_git_config_is_locked(tmp_path: Path): + # Another git command, or make install in another worktree of the clone, + # can hold .git/config's lock. + environment = _environment(tmp_path) + environment["UV_STUB_LOG"] = str(tmp_path / "uv-calls") + repository = _source_tree(tmp_path / "repository") + _git(repository, environment, "init", "--quiet") + (repository / ".git" / "config.lock").write_text("") + + installed = _make(repository, environment, "install") + + assert installed.returncode == 0, installed.stderr + assert "core.hooksPath" in installed.stderr + assert (tmp_path / "uv-calls").read_text() == "sync --frozen --extra dev\n" + + def test_the_hooks_are_executable_in_git(tmp_path: Path): environment = _environment(tmp_path) listed = _git( From ac4e18a2b778750ac4b131f5a45a8e71b9e1c11c Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:12:00 +0400 Subject: [PATCH 6/8] Refuse hidden Python files, git's uncompared files and make overrides - .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 --- .githooks/pre-commit | 68 +++++++++++++++++++++------ .githooks/pre-merge-commit | 5 +- .githooks/turn-on.sh | 23 +++++---- docs/engineering/skills/github-prs.md | 18 ++++--- docs/generated/rules.md | 2 +- rules/registry.toml | 4 +- 6 files changed, 86 insertions(+), 34 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index bd070e3db..ce9489c42 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -4,13 +4,16 @@ # commits without conflicts, so the checks pull-request CI runs fail here # first. make install turns both on with git config core.hooksPath .githooks; # every worktree of the clone shares that setting and runs its own checkout's -# copy. git commit --no-verify skips them, git's cherry-pick, revert and -# rebase make commits without them, and CI's Quality checks job still runs -# the same checks. +# copy. git commit --no-verify skips them, git's cherry-pick, revert, rebase +# and am can make commits without them, and CI's Quality checks job still +# runs the same checks. # # make quality checks the working tree, so the hook first requires it to hold -# exactly what is being committed: no unstaged changes to tracked files and no -# untracked files outside .gitignore. The commit stops, saying why, when that +# exactly what is being committed: no unstaged changes to tracked files, no +# tracked files git doesn't compare (skip-worktree or assume-unchanged), no +# untracked files outside .gitignore, and no untracked Python files at all +# outside the repository's .gitignore files, since mypy and Import Linter read +# those whatever else git ignores. The commit stops, saying why, when that # isn't so, when make quality fails, when ruff's fixes changed a file, or when # make, uv or the worktree's dev environment is missing. set -u @@ -25,8 +28,8 @@ root=$(git rev-parse --show-toplevel) || cd "$root" || fail "can't enter $root." # make's own variables from the environment could make it ignore a failing -# check or read another Makefile. -unset MAKEFLAGS MFLAGS GNUMAKEFLAGS MAKEFILES MAKELEVEL +# check, read another Makefile, or replace $(MAKE) and so skip quality-check. +unset MAKEFLAGS MFLAGS GNUMAKEFLAGS MAKEFILES MAKELEVEL MAKE MAKE_COMMAND MAKEOVERRIDES command -v make >/dev/null 2>&1 || fail "make isn't installed, so make quality can't run. Install make, then commit again." @@ -36,20 +39,57 @@ environment=${UV_PROJECT_ENVIRONMENT:-.venv} [ -d "$environment" ] || fail "this worktree has no dev environment ($environment). Run make install here, then commit again." -# Names as they are, not quoted as git quotes names outside ASCII. -unstaged=$(git -c core.quotePath=false diff --name-only) +# git -c core.quotePath=false shows names outside ASCII as they are. +unstaged=$(git -c core.quotePath=false diff --name-only) || + fail "git diff failed, so the hook can't tell what make quality would check." [ -z "$unstaged" ] || fail "make quality checks the working tree, which holds changes this commit doesn't. Stage them, or set them aside with git stash push --keep-index, then commit again: $unstaged" -untracked=$(git -c core.quotePath=false ls-files --others --exclude-standard) -[ -z "$untracked" ] || - fail "make quality checks the working tree, which holds files git doesn't track. Add them, remove them, or list them in .git/info/exclude, then commit again: -$untracked" + +newline=' +' +# git ls-files -v marks skip-worktree files S and assume-unchanged ones with a +# lowercase letter. (A loop, not a command substitution: bash 3.2, macOS's +# /bin/sh, misreads a case pattern's parenthesis inside one.) +flags=$(git -c core.quotePath=false ls-files -v) || + fail "git ls-files failed, so the hook can't tell what make quality would check." +uncompared= +while IFS= read -r entry; do + case $entry in + [a-zS]\ *) uncompared="$uncompared${uncompared:+$newline}${entry#??}" ;; + esac +done </dev/null) || exit 0 -if [ "$(cd "$root" && pwd -P)" != "$here" ]; then - echo "make install: $here isn't the root of a git working tree ($root is), so the pre-commit hook isn't turned on." >&2 +prefix=$(git rev-parse --show-prefix 2>/dev/null) || exit 0 +if [ -n "$prefix" ]; then + echo "make install: $(pwd) isn't the root of a git working tree (it is $prefix inside one), so the pre-commit hook isn't turned on." >&2 exit 0 fi @@ -24,8 +24,9 @@ if [ -n "$current" ]; then exit 0 fi +# -L follows symbolic links, both a linked hooks directory and linked hooks. hooks=$(git rev-parse --git-path hooks) -own=$(find "$hooks" -type f ! -name '*.sample' 2>/dev/null) +own=$(find -L "$hooks" -mindepth 1 ! -type d ! -name '*.sample' 2>/dev/null) if [ -n "$own" ]; then echo "make install: the pre-commit hook isn't turned on, because git would then stop running these hooks of the clone's own: $own @@ -33,5 +34,9 @@ Move them into .githooks, or call .githooks/pre-commit from them, then run git c exit 0 fi -git config core.hooksPath .githooks && +if git config core.hooksPath .githooks; then echo "make install: turned on the pre-commit hook for every worktree of this clone (core.hooksPath = .githooks)." +else + echo "make install: git couldn't set core.hooksPath (is .git/config locked by another git command?), so the pre-commit hook isn't turned on. Run make install again, or git config core.hooksPath .githooks." >&2 +fi +exit 0 diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 74348841b..5b3679cbf 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -209,8 +209,13 @@ the files and saying what to do: with `git stash push --keep-index`. This covers fixes `make quality` made in an earlier run, and `git commit ` while other files are changed; -- when there are untracked files outside `.gitignore`: add them, remove - them, or list them in `.git/info/exclude`; +- when there are untracked files outside the ignore files, or untracked + Python files outside the repository's `.gitignore` files: add them or + remove them. mypy and Import Linter read Python files on disk even when + `.git/info/exclude` or a global excludes file hides them; +- when tracked files are marked skip-worktree or assume-unchanged, which + `git diff` doesn't compare: clear the marks with + `git update-index --no-skip-worktree --no-assume-unchanged`; - when a check fails; - when ruff's fixes changed a file: review the fixes, stage them and commit again; @@ -221,10 +226,11 @@ the files and saying what to do: hooks, each from its own checkout; a worktree on a branch from before the hooks runs none. `make install` leaves `core.hooksPath` alone, and says why, when it would change an enclosing repository, replace another -`core.hooksPath`, or stop the clone's own hooks in `.git/hooks`. git's -cherry-pick, revert and rebase make commits without hooks. Don't skip them -with `git commit --no-verify`; CI's `Quality checks` job runs the same -checks and blocks the merge anyway. +`core.hooksPath`, or stop the clone's own hooks in `.git/hooks`, and when +git can't write the setting; it never stops `make install`. git's +cherry-pick, revert, rebase and am can make commits without hooks. Don't +skip them with `git commit --no-verify`; CI's `Quality checks` job runs the +same checks and blocks the merge anyway. `make quality` applies ruff's fixes (`ruff check --fix`, then `ruff format`), then runs `make quality-check`: `uv lock --check`, `ruff check`, diff --git a/docs/generated/rules.md b/docs/generated/rules.md index cc5db7488..e6bd5d7f5 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -377,7 +377,7 @@ flowchart LR | `api.quality.file-length` | A tracked Python file has at most 800 physical lines, or at most 1,500 if it is test code (a file under a tests or fixtures directory, or a conftest.py); a longer file is split by responsibility into modules, and keeps its public import path. | metric | enforced | yes | quality guard `file-length` | `pr.yml:quality-guards` | yes | | `api.quality.make-quality` | make quality is the command for the quality checks, locally and in CI: it applies ruff check --fix and ruff format, then runs make quality-check, which runs uv lock --check, ruff check, ruff format --check, Import Linter, mypy and the quality guards without changing files. Run with a stand-in uv, make quality runs each of them and fails whenever one fails, and make -n prints the same commands. pr.yml's quality-guards job runs make quality and then git diff --exit-code HEAD, with neither step skipped, allowed to fail, or run in another shell or directory, no MAKEFLAGS, GNUMAKEFLAGS or MAKEFILES set for them, and no earlier step writing to GITHUB_ENV or GITHUB_PATH; the Makefile uses no syntax the rules registry doesn't follow and nothing that hides a failing command; and no other pr.yml job's commands run ruff, mypy, lint-imports or the quality guards, directly or through a make target. | mechanical | enforced | yes | pytest `tests/unit/test_make_quality.py` | `pr.yml:test` | yes | | `api.quality.mypy-v2` | Shared query models, FastAPI dependencies, v2 routes and services, the gzip negotiation, the user sign-in and account-deletion libraries, the country list and its startup check pass mypy with untyped definitions disallowed. | mechanical | enforced | yes | mypy | `pr.yml:quality-guards` | — | -| `api.quality.pre-commit-hook` | make install turns on the git hooks in .githooks through core.hooksPath, a setting of the clone that every worktree shares and resolves against its own checkout, unless that would change an enclosing repository, replace another core.hooksPath, or stop the clone's own hooks, which it says. pre-commit runs make quality from the root of the working tree before git commit, and pre-merge-commit runs it before a merge commits without conflicts. Because make quality checks the working tree, the hook stops the commit when the working tree holds unstaged changes to tracked files or untracked files outside .gitignore, when make quality fails, when ruff's fixes changed a file, or when make, uv or the dev environment is missing, naming the files or what to install. | mechanical | enforced | yes | pytest `tests/unit/test_pre_commit_hook.py` | `pr.yml:test` | yes | +| `api.quality.pre-commit-hook` | make install turns on the git hooks in .githooks through core.hooksPath, a setting of the clone that every worktree shares and resolves against its own checkout, unless that would change an enclosing repository, replace another core.hooksPath, or stop the clone's own hooks (files or symbolic links), which it says; it always exits 0. pre-commit runs make quality from the root of the working tree before git commit, and pre-merge-commit runs it before a merge commits without conflicts, with make's variables from the environment cleared. Because make quality checks the working tree, the hook stops the commit when the working tree holds unstaged changes to tracked files, skip-worktree or assume-unchanged files, untracked files outside the ignore files, or untracked Python files outside the repository's .gitignore files; when make quality fails; when ruff's fixes changed a file; or when make, uv or the dev environment is missing, naming the files or what to install. | mechanical | enforced | yes | pytest `tests/unit/test_pre_commit_hook.py` | `pr.yml:test` | yes | | `api.quality.ruff-format` | All Python files are formatted with ruff format. | mechanical | enforced | yes | ruff format | `pr.yml:quality-guards` | — | | `api.quality.ruff-lint` | All Python code passes ruff check with every ruff rule selected (ALL), with no ratchet, except the ignores in pyproject.toml: COM812 and ISC001, which conflict with ruff format, and D203 and D213, which conflict with the kept D211 and D212. | mechanical | enforced | yes | ruff check (ALL) | `pr.yml:quality-guards` | — | | `api.quality.runtime-evaluated-models-resolve` | Every package model is one ruff treats as runtime-evaluated (a base resolves, through the import in its own module, to a name in runtime-evaluated-base-classes, or is a same-module class that does), and every Pydantic model and SQLAlchemy mapper resolves its annotations once the package is imported. | mechanical | enforced | yes | pytest `tests/unit/test_runtime_evaluated_annotations.py` | `pr.yml:test` | **unproven** | diff --git a/rules/registry.toml b/rules/registry.toml index e5943abb4..755dbaa21 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -1198,9 +1198,9 @@ test = "tests/unit/services/test_microdata_weight_boundaries.py::test_rule_proof [[rule]] id = "api.quality.pre-commit-hook" category = "quality" -statement = "make install turns on the git hooks in .githooks through core.hooksPath, a setting of the clone that every worktree shares and resolves against its own checkout, unless that would change an enclosing repository, replace another core.hooksPath, or stop the clone's own hooks, which it says. pre-commit runs make quality from the root of the working tree before git commit, and pre-merge-commit runs it before a merge commits without conflicts. Because make quality checks the working tree, the hook stops the commit when the working tree holds unstaged changes to tracked files or untracked files outside .gitignore, when make quality fails, when ruff's fixes changed a file, or when make, uv or the dev environment is missing, naming the files or what to install." +statement = "make install turns on the git hooks in .githooks through core.hooksPath, a setting of the clone that every worktree shares and resolves against its own checkout, unless that would change an enclosing repository, replace another core.hooksPath, or stop the clone's own hooks (files or symbolic links), which it says; it always exits 0. pre-commit runs make quality from the root of the working tree before git commit, and pre-merge-commit runs it before a merge commits without conflicts, with make's variables from the environment cleared. Because make quality checks the working tree, the hook stops the commit when the working tree holds unstaged changes to tracked files, skip-worktree or assume-unchanged files, untracked files outside the ignore files, or untracked Python files outside the repository's .gitignore files; when make quality fails; when ruff's fixes changed a file; or when make, uv or the dev environment is missing, naming the files or what to install." rationale = "docs/engineering/skills/github-prs.md#commit-hygiene" -scope = [".githooks/pre-commit", "Makefile"] +scope = [".githooks/pre-commit", ".githooks/pre-merge-commit", ".githooks/turn-on.sh", "Makefile"] tier = "mechanical" status = "enforced" autofix = false From dc4ecd28c815f7c153e677c7875b304be75a4eef Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:44:21 +0400 Subject: [PATCH 7/8] Test the hook under a UTF-8 locale, hidden settings and case renames (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 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 --- tests/unit/test_pre_commit_hook.py | 101 +++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/tests/unit/test_pre_commit_hook.py b/tests/unit/test_pre_commit_hook.py index f0a842a00..6ef6a71e3 100644 --- a/tests/unit/test_pre_commit_hook.py +++ b/tests/unit/test_pre_commit_hook.py @@ -654,6 +654,107 @@ def test_make_install_still_installs_when_git_config_is_locked(tmp_path: Path): assert (tmp_path / "uv-calls").read_text() == "sync --frozen --extra dev\n" +def test_a_commit_passes_in_a_utf8_locale(tmp_path: Path): + # bash 3.2, macOS's /bin/sh, matches uppercase letters with [a-z] in + # UTF-8 locales, and git ls-files -v marks every ordinary file H. + environment = _environment(tmp_path) + environment["LANG"] = environment["LC_ALL"] = "en_US.UTF-8" + repository = _repository(tmp_path, environment, PASSING_QUALITY) + + result = _commit_a_change(repository, environment) + + assert result.returncode == 0, result.stderr + assert _ran(tmp_path) == 1 + + +@pytest.mark.parametrize("name", ["ruff.toml", "mypy.ini", "lib/pyproject.toml"]) +def test_tool_settings_hidden_by_local_excludes_stop_the_commit(name, tmp_path: Path): + # ruff and mypy read their settings from these files before + # pyproject.toml, whatever git ignores. + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + settings = repository / name + settings.parent.mkdir(parents=True, exist_ok=True) + settings.write_text("") + (repository / ".git" / "info" / "exclude").write_text(f"{name}\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert name.split("/")[0] in result.stderr + assert _ran(tmp_path) == 0 + + +def test_a_hidden_directory_of_python_files_stops_the_commit(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "lib").mkdir() + (repository / "lib" / "helper.py").write_text("HELPER = 0\n") + (repository / ".git" / "info" / "exclude").write_text("lib/\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "lib/" in result.stderr + + +def test_a_hidden_virtual_environment_doesnt_stop_the_commit(tmp_path: Path): + # No check reads a virtual environment's files. + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "venv" / "lib").mkdir(parents=True) + (repository / "venv" / "pyvenv.cfg").write_text("home = /usr/bin\n") + (repository / "venv" / "lib" / "site.py").write_text("SITE = 0\n") + (repository / ".git" / "info" / "exclude").write_text("venv/\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode == 0, result.stderr + assert _ran(tmp_path) == 1 + + +def test_tracked_python_files_local_excludes_match_stop_the_commit(tmp_path: Path): + # ruff skips files .git/info/exclude matches, even tracked ones, which + # CI checks. + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / ".git" / "info" / "exclude").write_text("other.py\n") + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "other.py" in result.stderr + assert "ruff" in result.stderr + assert _ran(tmp_path) == 0 + + +def test_a_case_only_rename_stops_the_commit(tmp_path: Path): + # On a file system that ignores case, git status shows nothing, but the + # checks would read Module.py while the commit holds module.py. + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "other.py").rename(repository / "Other.py") + + result = _commit_a_change(repository, environment) + + assert result.returncode != 0 + assert "Other.py" in result.stderr or "other.py" in result.stderr + assert _ran(tmp_path) == 0 + + +def test_a_commit_of_some_paths_says_how_to_commit_the_rest(tmp_path: Path): + environment = _environment(tmp_path) + repository = _repository(tmp_path, environment, PASSING_QUALITY) + (repository / "other.py").write_text("OTHER = 1\n") + _git(repository, environment, "add", "other.py") + (repository / "module.py").write_text("VALUE = 2\n") + + result = _git(repository, environment, "commit", "-m", "module", "module.py") + + assert result.returncode != 0 + assert "without paths" in result.stderr + + def test_the_hooks_are_executable_in_git(tmp_path: Path): environment = _environment(tmp_path) listed = _git( From fc36ee404a4713b88442cd64916046c48df9d24f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:47:25 +0400 Subject: [PATCH 8/8] Work under UTF-8 locales, and refuse hidden settings and case renames - .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 --- .githooks/pre-commit | 105 ++++++++++++++++++-------- README.md | 3 +- docs/engineering/skills/github-prs.md | 22 ++++-- docs/generated/rules.md | 2 +- rules/registry.toml | 16 +++- 5 files changed, 107 insertions(+), 41 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index ce9489c42..4cc2a51fb 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -9,13 +9,15 @@ # runs the same checks. # # make quality checks the working tree, so the hook first requires it to hold -# exactly what is being committed: no unstaged changes to tracked files, no -# tracked files git doesn't compare (skip-worktree or assume-unchanged), no -# untracked files outside .gitignore, and no untracked Python files at all -# outside the repository's .gitignore files, since mypy and Import Linter read -# those whatever else git ignores. The commit stops, saying why, when that -# isn't so, when make quality fails, when ruff's fixes changed a file, or when -# make, uv or the worktree's dev environment is missing. +# exactly what is being committed: no unstaged changes to tracked files +# (including case-only renames), no tracked files git doesn't compare +# (skip-worktree or assume-unchanged), no untracked files outside the ignore +# files, no Python files or tool settings that only .git/info/exclude or a +# global excludes file hides (virtual environments aside), and no tracked +# Python files those excludes match, which ruff would skip. The commit stops, +# saying why, when that isn't so, when make quality fails, when ruff's fixes +# changed a file, or when make, uv or the worktree's dev environment is +# missing. set -u fail() { @@ -39,24 +41,29 @@ environment=${UV_PROJECT_ENVIRONMENT:-.venv} [ -d "$environment" ] || fail "this worktree has no dev environment ($environment). Run make install here, then commit again." -# git -c core.quotePath=false shows names outside ASCII as they are. -unstaged=$(git -c core.quotePath=false diff --name-only) || +# git -c core.quotePath=false shows names outside ASCII as they are, and +# core.ignorecase=false shows a case-only rename on a file system that ignores +# case as an untracked file. +names='git -c core.quotePath=false -c core.ignorecase=false' +newline=' +' + +unstaged=$($names diff --name-only) || fail "git diff failed, so the hook can't tell what make quality would check." [ -z "$unstaged" ] || - fail "make quality checks the working tree, which holds changes this commit doesn't. Stage them, or set them aside with git stash push --keep-index, then commit again: + fail "make quality checks the working tree, which holds changes this commit doesn't. Stage exactly what this commit needs and run git commit without paths, or set the rest aside with git stash push --keep-index, then commit again: $unstaged" -newline=' -' # git ls-files -v marks skip-worktree files S and assume-unchanged ones with a -# lowercase letter. (A loop, not a command substitution: bash 3.2, macOS's -# /bin/sh, misreads a case pattern's parenthesis inside one.) -flags=$(git -c core.quotePath=false ls-files -v) || +# lowercase letter. [[:lower:]], not [a-z]: bash 3.2, macOS's /bin/sh, matches +# uppercase letters with [a-z] in UTF-8 locales. (A loop, not a command +# substitution: bash 3.2 misreads a case pattern's parenthesis inside one.) +flags=$($names ls-files -v) || fail "git ls-files failed, so the hook can't tell what make quality would check." uncompared= while IFS= read -r entry; do case $entry in - [a-zS]\ *) uncompared="$uncompared${uncompared:+$newline}${entry#??}" ;; + [[:lower:]S]\ *) uncompared="$uncompared${uncompared:+$newline}${entry#??}" ;; esac done </dev/null)" ] || + continue + ;; + *.py | *.pyi | pyproject.toml | */pyproject.toml | setup.cfg | */setup.cfg | \ + mypy.ini | */mypy.ini | .mypy.ini | */.mypy.ini | ruff.toml | */ruff.toml | \ + .ruff.toml | */.ruff.toml | .importlinter | */.importlinter) ;; + *) continue ;; esac + hidden="$hidden${hidden:+$newline}$path" done <