Repository navigation
feat(commit): run pre-commit and commit-msg hooks on commit and amend - #84
Conversation
libgit2 never executes Git hooks, so every commit made in the app behaved like git commit --no-verify. Commit and amend now run pre-commit and commit-msg through `git hook run` (Git 2.36 or newer), found on the same augmented PATH as the AI CLIs and started through proc::hidden. - Hooks come from core.hooksPath or the common hooks folder, plus hooks defined in config; *.sample and non-executable files never count. With no hook present no git process starts. - The index is reloaded after pre-commit so re-staged files are committed. - commit-msg edits COMMIT_EDITMSG in the worktree's git dir and the edited message is committed. - Without a usable git the commit goes through with a warning. - The commit box gains a Skip hooks checkbox (only when the repository has hooks) and a dialog that shows a failed hook's output. pre-push, the post-hooks and hooks for other in-app commits come later. Refs cheat2001#82
The dialog's own X button is also named Close, so the unscoped locator matched two buttons and Playwright's strict mode failed the test.
git commit exports GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE before pre-commit runs, so a hook that validates the author failed in AngKorGit while the same native commit passed. Both commit hooks now get the commit's author, and amend passes the original author it keeps. The date uses git's raw "@<seconds> <+hhmm>" form.
…commit A hook that failed after switching tabs opened its dialog in the other repository, and "Commit without hooks" then committed that repository's draft with verification off. The finished commit also refreshed the graph with the old repository's history. A commit now carries its repository, message and amend flag from start to finish. The failure dialog keeps that request and its retry re-runs it, a failure for a repository that is no longer shown becomes a toast with no retry, status and graph refresh only while that repository is current, and the committing state is tracked per repository.
cheat2001
left a comment
There was a problem hiding this comment.
Thanks Chris, I read through the engine side and it holds up: index reload after pre-commit, COMMIT_EDITMSG in the worktree gitdir, signing untouched, amend byte for byte when there are no hooks. The tests are great.
One thing before I merge. The in-flight commit and the hook failure live in WorkingCopyPanel state, and the inspector swaps that panel for CommitDetails as soon as a commit is selected in the graph. So if I click Commit, lint-staged runs for 20 seconds and I click a commit meanwhile, a failing hook shows nothing at all. Coming back, Commit is enabled again and I can start a second hook run on the same index. The tab switch handling doesn't catch it because the panel stays mounted there. Could you move the committing set and the pending failure into a small store keyed by repo path, the way features/ai/workStore.ts does for reviews? An e2e that selects a graph commit while the hook runs would pin it.
Smaller things, fine as follow-ups:
- a successful commit clears the draft unconditionally, so anything typed during a long hook is lost. Clearing only when the draft still equals the sent message fixes it.
- after "Commit without hooks" the button still says "Running hooks…".
- the timeout kills git but not the hook it started, so lint-staged can keep writing the index after we've reported a failure.
- CLAUDE.md says Git for Windows ignores X_OK, but its is_executable actually looks for a #! line or .exe.
Upstream added its own G54 (WebKit focus), so the hooks gotcha is now G55. Both changelog entries and both sets of e2e coverage are kept.
The hook timeout killed git but not the hook git had started, so a hook such as lint-staged could keep writing the index after AngKorGit had already reported the failure. Hooks now run in their own process group on unix and a timeout kills the whole group; on Windows taskkill /F /T stops the tree. ai_cli gains spawn_tree and capture_tree for this, plain capture is unchanged, and libc is a unix-only dependency for killpg. Refs cheat2001#82
…anel The inspector swaps the commit box for the commit details as soon as a graph commit is selected. The running commit and its hook failure lived in the commit box, so a failure that landed meanwhile showed nothing and Commit was enabled again for a second hook run on the same index. A small store keyed by repository now holds the running commit and the skip-hooks choice, and pending hook failures live in a store with one dialog host mounted in App, so the dialog appears whatever the inspector shows. A failure for a repository that is not on screen waits there. Also from the review: - a successful commit clears the draft only when it still equals the message that was sent, so text typed during a long hook survives - "Running hooks…" shows only while the run actually verifies, so "Commit without hooks" no longer claims to run them - CLAUDE.md now cites where Git for Windows decides a hook exists Refs cheat2001#82
Brings in the cheat2001#84 review fixes and the upstream main merge: - the hooks gotcha is G55 now (upstream took G54) - hook failures from commit, amend and push share one store of pending failures keyed by repository; the commit box uses the commitWork store - both hook runners spawn the hook in its own process group and a timeout kills the whole group, including the direct runner's sh fallback
|
Thanks for the review! Everything is in
#85 is updated on top of these changes. |
Going back to the repository opens its waiting hook failure dialog at once, and the modal dialog hides the rest of the page from role queries, so the tab could not be found until the dialog was closed.
Checkout stays available while commit hooks run, and commit and amend read HEAD and its parents only after the hooks, then wrote through "HEAD". Checking out another branch at the same tip during a hook made the commit land on that branch. Commit and amend now record HEAD's branch, its commit and the repository state before the hooks and refuse to write when any of them changed. Refs cheat2001#82
The commit run checked that its repository was still on screen before awaiting the status refresh, so a switch during that await still let the graph reload with the old repository's history. It now checks again right before the reload. Refs cheat2001#82
The capture joined its stdout and stderr readers after the process exited. A hook that starts a background process without redirecting it keeps those pipes open, so the join waited for that process and Commit or Push never finished. The readers now stream into shared buffers, and after the process exits they get at most one second before the capture returns what was read. Refs cheat2001#82
cheat2001
left a comment
There was a problem hiding this comment.
Thanks Chris, that covers it. The new store, the dialog showing over the commit details and the draft only clearing when it's still the sent message all work, and the hook e2e tests pass in WebKit too. The background process fix is right as well, I ran your new test against the old wait and it sat there for the full 30 seconds. And you're right about Git for Windows, I mixed up is_executable with the hook lookup. Merging.
What & why
libgit2 never executes Git hooks, so every commit made in AngKorGit behaved like
git commit --no-verify. This is part 1 of #82: commit and amend now runpre-commitandcommit-msgthrough the Git CLI.In scope
pre-commitandcommit-msgare found in.git/hooksor incore.hooksPath, which counts from the worktree root and expands~. Hooks defined in config (hook.<name>.event, Git 2.54+) are detected too.*.sampleand non-executable files never count.git hook run --ignore-missing, which needs Git 2.36 or newer.proc::hidden(G31).git commit -mgives them:GIT_INDEX_FILE,GIT_EDITOR=:, and the commit's author inGIT_AUTHOR_NAME,GIT_AUTHOR_EMAILandGIT_AUTHOR_DATE(git's raw@<seconds> <±hhmm>form). Amend passes the original author it keeps. InheritedGIT_DIR-style variables are cleared first.hooks::runhelper is whatpre-pushand the post-hooks will reuse.<gitdir>/COMMIT_EDITMSG, which is per worktree. An edited message is committed after Git's whitespace cleanup, an unchanged one stays byte for byte, and an emptied one aborts the commit.commit-msgsees the current message.--no-verify). It only appears when the repository has commit hooks and clears after each commit.taskkill /F /Ton Windows), not justgit.Deferred
pre-push(next PR)prepare-commit-msg,post-commit,post-checkout,post-merge,post-rewritepost-merge(Start page: group context menu with fetch all and fast-forward-only pull #69):group_pull_matches_toolbar_pull_with_active_hooksis unchanged and still asserts that post-hooks do not run.Tests
13 engine tests in
tests/git_engine.rs:pre-commitpre-committhat re-stages a file (this test fails when the index reload is removed)commit-msgthat edits the message, leaves it untouched, fails, or empties itcore.hooksPathwins over.git/hooks; samples and non-executable files are ignoredcommit-msgin a linked worktreeThey skip with a note when Git 2.36 or newer is not on PATH.
6 module tests: Git version parsing (including the Apple and Windows forms), the author date format, message cleanup, output trimming, the
hook_failederror payload, and a timed-out hook's whole process tree being killed (unix; it fails when only the child is killed).5 e2e tests on demo mode (
angkorgit-demo-hooks,angkorgit-demo-hooks-delay): the failure dialog and Skip hooks, no checkbox without hooks, a hook that fails after a tab switch waiting in its repository, a running hook surviving a graph-commit selection with its failure still shown, and "Commit without hooks" never showing "Running hooks…" while text typed during the run is kept. The Playwright browsers are not installed on my machine, so I checked these flows by hand in demo mode; CI runs the tests. The first three also passed on Linux at 28f70a6, see below.Tested on Linux
Debian forky/sid, Git 2.53.0, Node 24.21, rustc 1.97.1, at 28f70a6:
pnpm typecheckpasses, andpnpm testpasses all 287 unit tests.cargo fmt --checkandcargo clippy --all-targets -- -D warningsare clean.cargo testpasses all 87 module tests and 137 engine tests. The 13 hook tests ran against Git 2.53, and none skipped.pnpm test:e2epasses all 124 tests.sign.rsmodule tests fail with the globalcommit.gpgsign=trueon that machine, and pass with a clean HOME.Screenshots
Taken on demo mode, dark theme; to be attached.
Checklist
pnpm typecheck && pnpm testpasscargo fmt --check && cargo clippy --all-targets -- -D warnings && cargo testpass (if Rust changed). Threesign.rsmodule tests fail locally with a globalcommit.gpgsign=trueand fail onmaintoo; everything passes with a clean HOME.tests/git_engine.rstracked()wrapper where undoableCLAUDE.mdupdated if conventions/architecture changedCo-authored-by: <tool>,Generated with …)