From a1aa4bedc6b92a4e8702f57519eafdba65af2b5b Mon Sep 17 00:00:00 2001 From: Karthik Suresh <7954591+k21993@users.noreply.github.com> Date: Thu, 6 Aug 2026 19:14:30 -0700 Subject: [PATCH] fix(hooks): stop lint.sh from modifying or discarding working tree changes lint.sh formats in place and then runs `git checkout -- $CHANGED` to undo the formatting. That restores from HEAD, so if a touched file also has uncommitted edits, those edits are discarded along with the formatting changes. The hook is run by contributor workflows such as alignment and pre-submit checks, so this can happen without the author explicitly choosing to format files. Unformatted files already on main make it easy to hit in practice. The same step also left the tree dirty: the restore filtered to *.py, but this ruff version formats Python blocks inside Markdown, so env README files were rewritten and never restored. Ask the tools for status instead of writing: `usort check` and `ruff format --check`. Nothing is modified, so nothing needs restoring, and the failure mode changes from mutating or discarding work to reporting the problem. The gate itself is unchanged, including its scope, so an unformatted tree still fails; whether envs/ belongs in that scope is a separate question left alone here. Tests run the real hook against a throwaway git repo with a stub uv on PATH, so they are hermetic and invoke no formatter. Both failing cases are pinned: an uncommitted edit must survive a run, and a clean tree must stay clean. --- .claude/hooks/lint.sh | 20 ++-- tests/scripts/test_lint_hook.py | 174 ++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 11 deletions(-) create mode 100644 tests/scripts/test_lint_hook.py 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) == []