Add pre-commit hook failure dialog with bypass option#20939
Add pre-commit hook failure dialog with bypass option#20939mosherc wants to merge 4 commits intodesktop:developmentfrom
Conversation
Introduces UI and logic to detect pre-commit hook failures and display a dialog showing the hook output. Users can choose to bypass the hook and commit with --no-verify. Updates commit flow, popup models, and styles to support this feature.
app/src/ui/pre-commit-hook-failure/pre-commit-hook-failure-dialog.tsx
Outdated
Show resolved
Hide resolved
app/src/lib/stores/app-store.ts
Outdated
| // Check for pre-commit hook failure indicators (case insensitive) | ||
| return output.includes('husky') || | ||
| output.includes('pre-commit') || | ||
| output.includes('lint-staged') |
There was a problem hiding this comment.
This will work for standard husky/lint-staged usage, and it should work for most custom pre-commit hooks assuming these words exist somewhere in the error. For example, my company has internal tooling for all this, and it caught lint-staged but not the other two, while my dummy repo with off-the-shelf usage catches all three basically. I figured not being too specific here for avoiding false negatives while being specific enough with tool names to avoid false positives is best for an initial attempt at this.
There was a problem hiding this comment.
In this case, I think false negatives are better than false positives, as a false positive opens the door to making a commit that bypasses the pre-commit hook with --no-verify.
Could you add a comment with sample strings found in the git output that basically led you to decide these three output.includes("…") ?
|
tested and seems to work. Screen.Recording.2025-09-03.at.9.35.13.AM.mov
|
Co-authored-by: Cory Baker <coryb08@gmail.com>
|
@niik would you be able to review this please? |
|
@mxie or @sergiou87 are you able to review? |
sergiou87
left a comment
There was a problem hiding this comment.
Thanks for your contribution! 💖
We still have to decide if we want to go ahead with it but I gave it an initial look at the implementation. My main concern is around the heuristics to detect whether or not this is a hook error.
app/src/ui/pre-commit-hook-failure/pre-commit-hook-failure-dialog.tsx
Outdated
Show resolved
Hide resolved
app/src/ui/pre-commit-hook-failure/pre-commit-hook-failure-dialog.tsx
Outdated
Show resolved
Hide resolved
app/src/ui/pre-commit-hook-failure/pre-commit-hook-failure-dialog.tsx
Outdated
Show resolved
Hide resolved
app/src/ui/pre-commit-hook-failure/pre-commit-hook-failure-dialog.tsx
Outdated
Show resolved
Hide resolved
app/src/lib/stores/app-store.ts
Outdated
| // Check for pre-commit hook failure indicators (case insensitive) | ||
| return output.includes('husky') || | ||
| output.includes('pre-commit') || | ||
| output.includes('lint-staged') |
There was a problem hiding this comment.
In this case, I think false negatives are better than false positives, as a false positive opens the door to making a commit that bypasses the pre-commit hook with --no-verify.
Could you add a comment with sample strings found in the git output that basically led you to decide these three output.includes("…") ?
Move the 'commit anyway' logic from App to PreCommitHookFailureDialog, allowing the dialog to handle bypassing pre-commit hooks directly. Simplify dialog state management and remove unnecessary props and code from App.
Improves formatting of the createCommit call for readability and updates pre-commit hook failure detection logic. The extractHookOutput function is simplified to only handle GitError instances, removing recursive and metadata error handling.
|
@sergiou87 what can we do to move this or some solution forward? This is a 5 year old highly requested feature, so it would be nice to get this or some alternative in. Most other ideas were turned down in the issue. |
|
@sergiou87 please review again when you get a chance. |
|
👋 @mosherc Sorry for the delayed response, we have been working on other stuff lately, but we keep your PR in mind. We are gonna chat about it next week and let you know. Can't promise anything, though 😅 |
Hey @mosherc, thanks for all your work putting this together. We fully agree with your premise but we feel conflicted on the solution. Git hooks have been one of our biggest sources of bug reports and complaints for a long time now but as you've seen it's been anything but easy to come up with a solution that covers all the edge cases we want to solve and we didn't want to head down a path that would later paint us into a corner due to not understanding the core of the issue to begin with. I decided to do a deeper dive on this during our bug bash week a few weeks back and that resulted in #21319. As someone who's given this a lot of thought I would love for you to pull those changes down and see if they work for your use case or not, that would help give us confidence in shipping something which I hope would be beneficial for you and others. In the meantime we don't want to go down the path of using heuristics to attempt to determine if a commit failed due to a hook so I'm going to close this out. We do appreciate the effort you put in here, it helped guide us towards what we ultimately hope is a good foundation for improving our hook support. |
|
@niik Thanks for sharing your thoughts. Honestly I was just going on comments in the original issue that suggested this might work, but my research did not lead to something that confirmed this was the best solution, just trying to find something to get the ball rolling. So even if that results in closing this and reviewing your alternative, I am quite happy with that! Just at a first glance, I love that it allows proactive hook skipping as well as reactive. I will continue the conversation there and test out your solution. Thanks! |
Closes #9351
Description
Adds support for bypassing pre-commit hooks when they fail during commit operations. When a pre-commit hook fails, GitHub
Desktop now displays a dialog showing the hook's error output and provides a "Commit anyway" button that uses
git commit --no-verifyto bypass the hooks.Key changes:
createCommit()function to acceptnoVerifyparameter and updatedICommitContextinterfaceexplicit hook mentions (case-insensitive)
PreCommitHookFailuredialog component with error styling, selectable text, and properaccessibility attributes
Design considerations:
positives
Screenshots
demo.mov
Release notes
Notes: Added support for bypassing pre-commit hooks when they fail. When a pre-commit hook prevents a commit, GitHub
Desktop now shows a dialog with the error output and an option to commit anyway using
--no-verify.