diff --git a/.claude/hooks/lint.sh b/.claude/hooks/lint.sh index cac612cb4..ade5d9751 100755 --- a/.claude/hooks/lint.sh +++ b/.claude/hooks/lint.sh @@ -19,20 +19,18 @@ if ! command -v uv &> /dev/null; then fi echo "=== Running import sort + format check ===" -# Run the same pipeline as arc f: usort then ruff format. -# If any file changes, the code wasn't properly formatted. -uv run usort format src/ tests/ >/dev/null 2>&1 -uv run ruff format src/ tests/ envs/ >/dev/null 2>&1 +# Ask the same tools as arc f whether the tree is formatted, without writing to +# it. Formatting in place and then running `git checkout --` to undo it also +# reverts the author's uncommitted edits in those files, and leaves reformatted +# Markdown behind, because the restore only ever covered *.py. +FORMAT_FAILED=0 +uv run usort check src/ tests/ || FORMAT_FAILED=1 +uv run ruff format --check src/ tests/ envs/ || FORMAT_FAILED=1 -# Check if any files were modified (means they weren't formatted before) -CHANGED=$(git diff --name-only -- '*.py' 2>/dev/null || true) -if [ -n "$CHANGED" ]; then - echo "ERROR: The following files need formatting:" - echo "$CHANGED" +if [ "$FORMAT_FAILED" -ne 0 ]; then echo "" + echo "ERROR: the files listed above need formatting." echo "Run: uv run usort format src/ tests/ && uv run ruff format src/ tests/ envs/" - # Undo the formatting so the working tree stays as-is - git checkout -- $CHANGED 2>/dev/null || true exit 1 fi echo "Import sort + format check passed!" diff --git a/tests/scripts/test_lint_hook.py b/tests/scripts/test_lint_hook.py new file mode 100644 index 000000000..27bd76c0b --- /dev/null +++ b/tests/scripts/test_lint_hook.py @@ -0,0 +1,174 @@ +# SPDX-License-Identifier: BSD-3-Clause + +"""`.claude/hooks/lint.sh` must never modify the working tree. + +The hook says so itself ("Undo the formatting so the working tree stays +as-is"), and `/alignment-review` and `/pre-submit-pr` run it automatically, so +an author can trigger it without meaning to. It previously formatted in place +and then ran `git checkout --` on whatever changed, which restores from HEAD: +that discarded the formatting *and* any uncommitted edits in the same files, +and left reformatted Markdown behind because the restore only covered `*.py`. + +These run the real hook against a throwaway git repo with a stub `uv` on PATH, +so no formatter is actually invoked and the test is hermetic. The stub rewrites +files in "format" mode and reports status in "check" mode, which is enough to +tell a hook that writes from one that only reports. +""" + +from __future__ import annotations + +import pathlib +import shutil +import subprocess + +import pytest + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] +LINT_HOOK = REPO_ROOT / ".claude" / "hooks" / "lint.sh" + +# The stub stands in for `uv`. In check mode it reports whether any target file +# still carries the marker; otherwise it rewrites them, standing in for a +# formatter that modifies files in place. +STUB_UV = """#!/bin/bash +mode=report +for a in "$@"; do + if [ "$a" = "--check" ] || [ "$a" = "check" ]; then mode=check; fi +done +targets=() +for a in "$@"; do + if [ -e "$a" ]; then targets+=("$a"); fi +done +[ ${#targets[@]} -eq 0 ] && exit 0 + +found=1 +while IFS= read -r f; do + if grep -q NEEDSFORMAT "$f" 2>/dev/null; then + found=0 + if [ "$mode" != "check" ]; then + sed -i.bak 's/NEEDSFORMAT/WASFORMATTED/g' "$f" && rm -f "$f.bak" + fi + fi +done < <(find "${targets[@]}" -type f \\( -name '*.py' -o -name '*.md' \\) 2>/dev/null) + +if [ "$mode" = "check" ] && [ $found -eq 0 ]; then exit 1; fi +exit 0 +""" + +MARKER = "def keep_me():\n return 'uncommitted work'\n" + + +@pytest.fixture +def repo(tmp_path: pathlib.Path) -> pathlib.Path: + """A git repo whose committed files are already 'unformatted'.""" + if shutil.which("git") is None: # pragma: no cover + pytest.skip("git is required") + + # The stub lives outside the repo so it cannot show up as untracked and + # confuse the "did the hook dirty the tree" assertions. + work = tmp_path / "repo" + (work / "src").mkdir(parents=True) + (work / "tests").mkdir() + (work / "envs" / "demo_env").mkdir(parents=True) + + # Committed in a state the formatter wants to change, mirroring the 55 + # unformatted files that sit under envs/ on main. + (work / "src" / "mod.py").write_text("# NEEDSFORMAT\nx = 1\n") + (work / "envs" / "demo_env" / "README.md").write_text( + "# Demo\n\n```python\n# NEEDSFORMAT\nx = 1\n```\n" + ) + + run = lambda *a: subprocess.run( # noqa: E731 + a, cwd=work, check=True, capture_output=True + ) + run("git", "init", "-q") + run("git", "config", "user.email", "t@example.com") + run("git", "config", "user.name", "t") + run("git", "add", "-A") + run("git", "commit", "-qm", "init") + + bin_dir = tmp_path / "stubbin" + bin_dir.mkdir() + stub = bin_dir / "uv" + stub.write_text(STUB_UV) + stub.chmod(0o755) + return work + + +def _run_hook(repo: pathlib.Path) -> subprocess.CompletedProcess[str]: + env = { + "PATH": f"{repo.parent / 'stubbin'}:/usr/bin:/bin:/usr/sbin:/sbin", + "HOME": str(repo.parent), + } + return subprocess.run( + ["bash", str(LINT_HOOK)], + cwd=repo, + env=env, + capture_output=True, + text=True, + ) + + +def _dirty(repo: pathlib.Path) -> list[str]: + out = subprocess.run( + ["git", "status", "--porcelain"], + cwd=repo, + capture_output=True, + text=True, + check=True, + ) + return [line for line in out.stdout.splitlines() if line.strip()] + + +def test_hook_preserves_uncommitted_python_edits(repo: pathlib.Path) -> None: + """The author's uncommitted work must survive a hook run. + + This is the data-loss case: `git checkout -- $CHANGED` restores from HEAD, + taking the edits with the formatting. + """ + target = repo / "src" / "mod.py" + target.write_text(target.read_text() + "\n" + MARKER) + + _run_hook(repo) + + assert "uncommitted work" in target.read_text(), ( + "lint.sh discarded uncommitted edits in src/mod.py. It formats in place " + "and then runs `git checkout --` on every changed file, which restores " + "from HEAD and takes the author's work with the formatting." + ) + + +def test_hook_leaves_no_files_modified(repo: pathlib.Path) -> None: + """A run on a clean tree must leave it clean, Markdown included. + + The old restore step filtered to `*.py`, so reformatted Markdown was left + behind on every run. + """ + assert _dirty(repo) == [] + + _run_hook(repo) + + assert _dirty(repo) == [], ( + "lint.sh modified tracked files. It must report formatting problems " + "without writing to the working tree." + ) + + +def test_hook_still_fails_when_formatting_is_needed(repo: pathlib.Path) -> None: + """Not writing must not mean not reporting: the gate still has to fail.""" + result = _run_hook(repo) + + assert result.returncode != 0 + assert "format" in (result.stdout + result.stderr).lower() + + +def test_hook_passes_when_everything_is_formatted(repo: pathlib.Path) -> None: + """And it must still succeed when there is nothing to fix.""" + for path in repo.rglob("*"): + if path.is_file() and path.suffix in {".py", ".md"}: + path.write_text(path.read_text().replace("NEEDSFORMAT", "ok")) + subprocess.run(["git", "commit", "-qam", "format"], cwd=repo, check=True) + + result = _run_hook(repo) + + assert result.returncode == 0, result.stdout + result.stderr + assert _dirty(repo) == []