Skip to content

fix(hooks): stop pre-commit and pre-push from rewriting uncommitted work - #1576

Merged
burtenshaw merged 2 commits into
openenv:mainfrom
yaokaiy:fix/hooks-keep-uncommitted-work
Nov 9, 2026
Merged

burtenshaw merged 2 commits into
openenv:mainfrom
yaokaiy:fix/hooks-keep-uncommitted-work

Conversation

@yaokaiy

@yaokaiy yaokaiy commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Part of #1575 (item 1).

The pre-commit and pre-push hooks written by install.sh format files in place before they call lint.sh. pre-commit then runs git add on every changed .py file, staging edits the author left unstaged. pre-push runs git checkout -- on them, which discards the edits.

lint.sh already checks formatting without writing (#1163), and both hooks call it. This PR removes the inline format step from both hooks.

Behavior change: pre-commit no longer formats and stages files for you. It reports them through lint.sh, as pre-push does.

Type of Change

  • Bug fix

Alignment Checklist

  • I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
  • I have checked .claude/docs/INVARIANTS.md and no invariants are violated
  • I have run /pre-submit-pr (or bash .claude/hooks/lint.sh and tests) and addressed all issues. No new findings; the lint.sh failures already on main are Git hooks discard uncommitted edits, and lint.sh fails on a clean main #1575 item 2.

RFC Status

  • Not required (bug fix, docs, minor refactoring)

Test Plan

  • test_git_hooks_leave_uncommitted_work_alone[pre-commit] and [pre-push] in tests/scripts/test_lint_hook.py install the hooks in the existing throwaway repo, stage one file, leave another edit unstaged, run the hook, and assert that the working tree and index are unchanged. Both fail on main and pass here.
  • The same scenario with the real uv, usort and ruff: on main, pre-push drops the uncommitted edit and pre-commit stages the unstaged file. On this branch neither happens.

Claude Code Review

/alignment-review: no Tier 1 issues; no principle, invariant or RFC conflicts.


Note

Low Risk
Developer workflow change only: hooks no longer auto-format; authors must fix formatting when lint fails.

Overview
Stops pre-commit and pre-push from running in-place usort/ruff format before lint.sh. Those steps had been mutating the tree: pre-commit auto-staged reformatted .py files (pulling in work the author meant to leave unstaged), and pre-push reformatted then git checkout --, which could discard uncommitted edits.

Both hooks now rely on lint.sh only for format and lint—same report-only behavior as /pre-submit-pr and /alignment-review. pre-commit no longer auto-fixes and stages for you; failures tell you to format manually.

Docs (CLAUDE.md, install.sh echo text) reflect the slimmer hook behavior. tests/scripts/test_lint_hook.py adds parametrized coverage that installed pre-commit and pre-push leave the index and working tree unchanged when there are staged and unstaged edits.

Reviewed by Cursor Bugbot for commit d6982bd. Bugbot is set up for automated code reviews on this repo. Configure here.

The installed hooks ran their own format-in-place step before calling
lint.sh. pre-commit then staged every changed .py file, including edits
left unstaged, and pre-push ran `git checkout --` on them, discarding the
edits. lint.sh already checks formatting without writing (openenv#1163), so drop
the inline step from both hooks.

Part of openenv#1575
@cursor cursor Bot mentioned this pull request Nov 9, 2026
2 tasks done

@burtenshaw burtenshaw left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge: both hooks stop staging or discarding uncommitted work, lint.sh still blocks the same problems, and the new tests fail on main and pass here.

Findings

Nothing blocking. There is one pre-existing problem in the same pre-push path, and one optional test change.

1. The test fixture can rewrite the developer's repository when pre-push runs it (pre-existing, follow-up). Git exports GIT_DIR to hooks that run from a linked worktree. pre-push runs the test suite through .claude/hooks/test.sh:17, and the repo fixture in tests/scripts/test_lint_hook.py:83-94 calls git with the inherited environment. Its git config and git commit calls then act on the developer's repository instead of the throwaway one. The developer ends up with a commit on their branch whose tree holds only src/mod.py and envs/demo_env/README.md. Their repo config also gets user.name t, user.email t@example.com and core.hooksPath /dev/null. I reproduced this with a real git push from a linked worktree, on main and on this branch. It needs timeout on PATH, so it affects Linux and macOS with GNU coreutils. On stock macOS, test.sh fails before pytest starts.

Adding one line before the pytest call in .claude/hooks/test.sh fixes it for the whole suite. Other tests that run git in a temporary directory also inherit the environment, for example tests/discovery/ and tests/scripts/test_prepare_hf_deployment.py.

unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE

With that line, the same push leaves the repository alone and all 6 tests pass. Clearing the same three variables with monkeypatch.delenv in the repo fixture also works, but it only covers this file. A follow-up PR is fine.

Reproduction
# From an OpenEnv checkout. Git sets GIT_DIR like this for a pre-push hook
# that runs from a linked worktree.
scratch=$(mktemp -d)
git init -q -b main "$scratch/repo"
git -C "$scratch/repo" commit -q --allow-empty -m "scratch init"
git -C "$scratch/repo" worktree add -q "$scratch/wt" -b feature
GIT_DIR="$scratch/repo/.git/worktrees/wt" PYTHONPATH=src:envs \
  uv run pytest tests/scripts/test_lint_hook.py -q    # 1 passed, 5 errors
git -C "$scratch/wt" log -1 --format='%s (%an)'        # init (t)
git -C "$scratch/repo" config core.hooksPath           # /dev/null

2. Optional: assert that the hooks still fail. Add assert result.returncode != 0 after the assertion at tests/scripts/test_lint_hook.py:227. The test then also shows that both hooks still block unformatted code. Today a hook that stopped calling lint.sh would pass it. The assertion passes on this branch and fails for pre-commit when the lint.sh call is removed.

What I checked

  • On main, in a throwaway repo with the stub uv from the tests: one file had a staged hunk, an unstaged hunk and a formatting problem, and an already formatted file under envs/ had an unstaged edit. pre-commit staged both files, and the commit went through with the unstaged edits in it. pre-push discarded both unstaged edits, including the one that needed no formatting. With this branch, both hooks fail and leave the index and working tree unchanged.
  • With real usort 1.1.3 and ruff 0.17.4, lint.sh fails on unsorted imports and on unformatted code in src/, tests/ and envs/, and writes nothing. Coverage does not drop, because .claude/hooks/lint.sh:27-28 checks everything the removed steps formatted. The change also removes a second side effect. On main, both hooks leave reformatted Markdown in the working tree, because ruff now formats .md files and the hooks only stage or restore *.py.
  • tests/scripts/test_lint_hook.py: 6 passed here. Against main's hooks, the two new cases fail and the other 4 pass. ruff format, ruff check and usort check are clean, and CI is green.
  • CLAUDE.md:313 and CLAUDE.md:315 match the hook bodies and the summary that install.sh prints. No other doc says pre-commit formats files.
  • lint.sh still fails on a clean checkout (#1575 item 2), so pre-commit blocks every commit with or without this PR.

@burtenshaw
burtenshaw merged commit 9a53819 into openenv:main Nov 9, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants