Skip to content

fix(hook): do not stage fixes when fail_on_fix=true#892

Merged
jdx merged 1 commit intomainfrom
claude/reverent-shaw-9d6102
Apr 30, 2026
Merged

fix(hook): do not stage fixes when fail_on_fix=true#892
jdx merged 1 commit intomainfrom
claude/reverent-shaw-9d6102

Conversation

@jdx
Copy link
Copy Markdown
Owner

@jdx jdx commented Apr 30, 2026

Summary

When a hook has fail_on_fix=true, the step's auto-staging would silently re-stage the fixer's output over the user's explicit git add choices. After the failed commit:

  • The user's staged changes were merged with / overwritten by the fix
  • The fix was no longer visible as an unstaged change for review
  • A re-commit would silently succeed with the fix baked in — defeating the entire point of fail_on_fix

This is the bug reported in #888.

Fix

Force should_stage = false during fix runs when fail_on_fix = true (src/hook.rs:828). The fixer's output stays in the worktree as an unstaged change, the user's staged changes are preserved, and a re-commit will fail again until the user explicitly accepts the fix.

After the fix, the user's reproduction scenario yields exactly what they expected:

=== git status AFTER failed commit ===
Changes to be committed:
        modified:   a.json        # user's original staged change preserved
Changes not staged for commit:
        modified:   a.json        # fixer's reformatting now visible for review
        modified:   b.md          # untouched

Test plan

  • New regression test in test/fail_on_fix.bats reproducing the discussion scenario
  • All 5 fail_on_fix.bats tests pass in both libgit2 and nolibgit2 modes
  • All other stage / stash bats tests still pass
  • cargo test (145 tests) passes
  • Manual reproduction matches expected behavior

Closes #888

🤖 Generated with Claude Code


Note

Medium Risk
Changes pre-commit/hook execution behavior around git staging during fix runs, which can affect users’ commit contents. Scope is small and covered by a new regression test, but it touches git index/worktree semantics.

Overview
Ensures fail_on_fix=true runs do not auto-stage fixer modifications by forcing should_stage off for RunType::Fix, so a failed hook leaves fixer edits as unstaged changes for review and does not overwrite the user’s staged selection.

Adds a Bats regression test reproducing #888 to verify a failed pre-commit preserves the original staged diff while exposing the fixer’s reformatting in the working tree.

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

When `fail_on_fix=true` is set on a hook, the step's auto-staging would
silently re-stage the fixer's output over the user's explicit `git add`
choices. After the failed commit the user couldn't see the proposed
fix as an unstaged change for review, and a re-commit would silently
succeed with the fix baked in — defeating the entire point of
`fail_on_fix`.

Force `should_stage=false` during fix runs when `fail_on_fix=true`. The
fixer's output stays in the worktree as an unstaged change, the user's
staged changes are preserved, and a re-commit will fail again until the
user explicitly accepts the fix.

Closes #888

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR fixes a bug (#888) where fail_on_fix=true hooks would silently auto-stage the fixer's output, overwriting the user's deliberate git add choices and allowing a re-commit to succeed without the user explicitly reviewing the fix. The fix is a single targeted condition in hook.rs that forces should_stage = false when both fail_on_fix is set and the run type is Fix, keeping the fixer's output as an unstaged diff for review.

Confidence Score: 5/5

This PR is safe to merge — it is a minimal, well-targeted fix with a solid regression test covering the exact bug scenario.

The change is two lines touching a single, narrow condition. The logic is correct: forcing should_stage=false when fail_on_fix=true && run_type==Fix has no unintended side effects. The bats test faithfully reproduces the original bug report and verifies both sides of the invariant. No security or data-integrity risks introduced.

No files require special attention.

Important Files Changed

Filename Overview
src/hook.rs Two-line fix: overrides should_stage to false when fail_on_fix=true and run_type=Fix, correctly preventing fixer output from being staged and defeating the purpose of fail_on_fix.
test/fail_on_fix.bats New regression test reproducing the exact issue from #888: verifies staged changes are preserved and the fixer's output remains as an unstaged diff after a failed commit.

Sequence Diagram

sequenceDiagram
    participant User
    participant Git
    participant HK as hk (pre-commit)
    participant Fixer

    User->>Git: git add a.json
    User->>Git: git commit -m "update"
    Git->>HK: run pre-commit hook
    Note over HK: fail_on_fix=true, run_type=Fix<br/>→ should_stage forced to false
    HK->>Fixer: run fix step on a.json
    Fixer-->>HK: a.json modified (spaces stripped)
    Note over HK: should_stage=false → skip git add
    HK-->>Git: fail (files were modified by fixer)
    Git-->>User: commit rejected

    Note over User,Git: After fix (correct behavior)
    User->>Git: git diff --cached a.json
    Git-->>User: shows user's original staged change (a.json with spaces)
    User->>Git: git diff a.json
    Git-->>User: shows fixer's unstaged change (spaces stripped)
Loading

Reviews (1): Last reviewed commit: "fix(hook): do not stage fixes when fail_..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request modifies the hook execution logic in src/hook.rs to prevent automatic staging of fixes when fail_on_fix is enabled, ensuring that fixes are surfaced for manual review. Additionally, a new integration test in test/fail_on_fix.bats verifies that existing staged changes are preserved and fixes appear as unstaged changes. I have no feedback to provide.

@jdx jdx merged commit 4187883 into main Apr 30, 2026
30 of 37 checks passed
@jdx jdx deleted the claude/reverent-shaw-9d6102 branch April 30, 2026 14:11
@jdx jdx mentioned this pull request Apr 30, 2026
jdx added a commit that referenced this pull request Apr 30, 2026
### 🐛 Bug Fixes

- **(hook)** do not stage fixes when fail_on_fix=true by
[@jdx](https://github.com/jdx) in
[#892](#892)
- use site domain for plausible data-domain by
[@jdx](https://github.com/jdx) in
[#886](#886)
- make text-mode progress output usable in CI by
[@jdx](https://github.com/jdx) in
[#890](#890)

### 📚 Documentation

- prefix GitHub star count with ★ glyph by
[@jdx](https://github.com/jdx) in
[#883](#883)

### 🔍 Other Changes

- **(release)** dedupe sponsor section in release notes by
[@jdx](https://github.com/jdx) in
[#881](#881)
- switch analytics from gtm/goatcounter to plausible by
[@jdx](https://github.com/jdx) in
[#885](#885)
- migrate to namespace.so runners by [@jdx](https://github.com/jdx) in
[#891](#891)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk release bookkeeping: version bumps, regenerated docs, and
lockfile dependency updates with no functional code changes in this PR.
> 
> **Overview**
> Bumps `hk` to **v1.44.3** and adds the corresponding `CHANGELOG.md`
release entry.
> 
> Regenerates versioned docs/CLI artifacts to reference `1.44.3`
(package URLs and generated `commands.json`/`index.md`) and updates
`Cargo.lock` with dependency resolution changes (notably `jni`,
`rustls*`, `reqwest`, `wasm-bindgen`, and `thiserror` unification).
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
ab7b72e. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: mise-en-dev <123107610+mise-en-dev@users.noreply.github.com>
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.

1 participant