Repository navigation
fix(hooks): stop lint.sh from mutating the tree (from #1070) - #1163
Conversation
…anges 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.
Mirror tests/discovery: disable commit.gpgsign and core.hooksPath so local signing/hooks cannot fail the throwaway repo (Bugbot on #1070). Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
burtenshaw
added
bug
Cursor
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Alignment Review Report
Scope (git diff 81ef28d6...64d1fd8d) — two files, dev-tooling only:
.claude/hooks/lint.sh— rewritten to check formatting without mutating the working treetests/scripts/test_lint_hook.py— new, 4 hermetic regression tests
No src/, environment, client, server, API, or wire-type code is touched.
Automated Checks
- Lint: PASS (for this PR's changes). The new
tests/scripts/test_lint_hook.pypasses all three checks standalone —usort check,ruff format --check,ruff check(clean).lint.shis Bash and isn't ruff-linted. Running the PR's ownlint.shacross the repo exits non-zero only on pre-existing formatting debt (tests/envs/test_julia_env.py,tests/envs/test_grid_world.pyfor usort;envs/README.mdand other env files for ruff-format) — none introduced here. - Debug code: CLEAN.
check-debug.shscanssrc/only (untouched by this PR); its hits are all pre-existing. The new test file has noprint/breakpoint/pdb/TODO.
Open RFCs Context
All RFCs are In Review (000, 001, 002, 003, 005, 008) or Draft (010, 011); 004 carries no explicit status line. Every one concerns environment/framework design (abstractions, env spec, MCP, rubrics, harnesses, validation, world-modeling, catalog discovery). None govern contributor/dev tooling such as git hooks or the lint pipeline, so no RFC is implicated.
Tier 1: Fixes Required
None.
- The Bash rewrite is correct and idiomatic. Under
set -e,uv run … || FORMAT_FAILED=1neutralizes the non-zero exit so bothusort checkandruff format --checkalways run (the author sees every problem in one pass) beforeexit 1. The trailingruff check src/ tests/is unchanged. - The new test is lint-clean and passes (4/4).
Tier 2: Alignment Discussion
Principle Conflicts — None identified. This is repo tooling; it doesn't touch rewards-in-environment, client/server separation, the Gymnasium reset/step/state surface, the MCP/WebSocket boundary, or Pydantic wire types (PRINCIPLES.md / INVARIANTS.md).
RFC Conflicts — None identified (see Open RFCs Context).
Verification performed
- The two new tests are genuine regressions. Reconstructed the pre-fix hook (
81ef28d6:.claude/hooks/lint.sh) against the new tests:test_hook_preserves_uncommitted_python_editsandtest_hook_leaves_no_files_modifiedfail (the oldgit checkout -- $CHANGEDdiscards the author's uncommitted edits, and the*.py-only restore leaves a reformatted file behind); all four pass against the new hook. - Live proof of the fix. Running the PR's
lint.shon this checkout leftgit statuscompletely clean — no tree mutation. (The old hook would have left ~30envs/**/README.mdmodified.) - The Markdown rationale checks out.
ruff format(0.16.7) reformats the Python code fences insideenvs/README.md; because the old restore filtered to*.py, those Markdown edits were genuinely left behind on every run.
Summary
- 0 mechanical issues to fix
- 0 alignment points for human review
- 0 RFC conflicts to discuss
Clean, tightly-scoped bug fix with high-signal regression coverage. It also removes a long-standing side effect where /alignment-review and /pre-submit-pr — which run this hook automatically — could silently reformat or discard an author's uncommitted work. Suggested reviewer: @Darktex (author of the original hook and the mutate-then-restore block being replaced); cc @burtenshaw (CI/lint tooling).
Sent by Cursor Automation: Pre-review
| # 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 |
There was a problem hiding this comment.
The core fix: --check replaces the old format-in-place + git checkout -- $CHANGED. Verified live — running this on the repo now leaves git status clean, whereas the old hook left ~30 envs/**/README.md modified.
Nice detail: || FORMAT_FAILED=1 keeps set -e from short-circuiting here, so both usort check and ruff format --check run and the author sees every issue in one pass (rather than stopping at the first failing tool).
| return [line for line in out.stdout.splitlines() if line.strip()] | ||
|
|
||
|
|
||
| def test_hook_preserves_uncommitted_python_edits(repo: pathlib.Path) -> None: |
There was a problem hiding this comment.
High-signal regression test. I ran it against the pre-fix hook and it fails — the old git checkout -- $CHANGED restores from HEAD and takes the uncommitted edits along with the formatting. test_hook_leaves_no_files_modified likewise fails on the old hook; both pass on the new one.
The stubbed-uv-on-PATH + throwaway-repo design keeps this hermetic (no real formatter, no network), so it runs cleanly in CI.
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
Summary
Replacement for #1070 (fork workflows stuck at
action_required). Same change: stoplint.shfrom modifying/discarding the working tree, plus a small hermeticity fix so the throwaway git fixture ignores globalcommit.gpgsign/core.hooksPath(Bugbot).Please close #1070 once this lands.
Type of Change
Alignment Checklist
mainso required CI can run on-repoRFC Status
Test Plan
tests/scripts/test_lint_hook.pyClaude Code Review
N/A — release automation replacement for Ben's watch/merge ask on #1070.
Note
Low Risk
Changes only the Claude lint hook and adds tests; no runtime product, auth, or data-path impact.
Overview
Fixes a contributor-tooling bug where
.claude/hooks/lint.shformatted files in place and then rangit checkout --on changed paths. That could wipe uncommitted edits in the same files and leave reformatted Markdown dirty because restore only targeted*.py.The hook now uses read-only checks (
usort check,ruff format --check) and fails with the same remediation message, without touching the working tree.Adds
tests/scripts/test_lint_hook.py: runs the reallint.shin a throwaway repo with a stubuv(check vs write modes) to assert the tree stays clean, uncommitted Python edits survive, failures still report formatting issues, and a formatted tree passes. The fixture disables globalcommit.gpgsign/core.hooksPathfor hermetic CI/local runs.Reviewed by Cursor Bugbot for commit be306ba. Bugbot is set up for automated code reviews on this repo. Configure here.