[PR #540] Improve hook handler quality: fix bugs, simplify, add tests #550

New issue

Closed

opened 2026-03-02 04:13:56 +03:00 by kerem · 0 comments

kerem commented

2026-03-02 04:13:56 +03:00

Owner

Original Pull Request: https://github.com/git-ai-project/git-ai/pull/540

State: closed
Merged: Yes

Fix bugs, simplify parsing, add unit tests for hook handlers

Summary

Follow-up quality improvements to the hook handlers introduced in PR #530. All changes in src/commands/git_hook_handlers.rs:

Bug fixes:

Non-managed hooks silently broken in hooks-only mode (Round 3): When git-ai takes over core.hooksPath for a repo, only MANAGED_GIT_HOOK_NAMES were installed as symlinks. Non-managed core hooks (e.g. commit-msg, pre-merge-commit) had no symlinks in the managed hooks directory, so pre-existing repo-level or global hooks for those events silently stopped executing. Now selectively installs symlinks (to the git-ai binary) for non-managed hooks only when the corresponding script actually exists in the forward target directory. This avoids unnecessary hook interception while preserving $0/dirname for Husky-style hooks that resolve relative paths. Stale symlinks are cleaned up when the original hook is removed.
handle_git_hook_invocation called should_forward_repo_state_first(None) twice per hook invocation (once to check existence, once inside execute_forwarded_hook). Now cached into cached_forward_dir and threaded through.
latest_cherry_pick_source_from_sequencer used is_valid_git_oid (requires exactly 40 or 64 hex chars) to validate entries in the sequencer done file, which can contain abbreviated SHAs. Added is_valid_git_oid_or_abbrev (≥7 hex chars) and use it there instead.
maybe_enable_rebase_hook_mask called SystemTime::now() twice — once for session_id and once for created_at_unix_ms — producing potentially inconsistent timestamps. Now computes once and reuses.
handle_rebase_post_rewrite_from_stdin: .last().unwrap_or_else(|| original_commits[0].clone()) had a dead-code panic fallback (vec is guaranteed non-empty by earlier is_empty() guard). Simplified to .last().unwrap().

Simplifications:

Extracted sync_non_managed_hook_symlinks to scan the forward target directory and selectively install/clean up non-managed hook symlinks. Extracted remove_hook_entry helper to deduplicate cleanup logic.
Extracted parse_whitespace_fields(stdin, min_fields) to consolidate the duplicated split_whitespace + filter logic between parse_hook_stdin and parse_reference_transaction_stdin.
Removed 20-line explicit no-op match arm in run_managed_hook ("commit-msg" | "pre-merge-commit" | ...) that was already covered by the _ => 0 wildcard.
Extracted is_null_oid helper to replace 5 scattered chars().all(|c| c == '0') patterns.
Rewrote hook_has_no_managed_behavior to derive from MANAGED_GIT_HOOK_NAMES constant instead of manually maintaining a 20-entry match list. Self-maintaining and can't drift.
Shared git_dir lookup in hook_requires_managed_repo_lookup reference-transaction arm (was calling git_dir_from_context() twice via separate helper functions).
Removed dead is_cherry_pick_in_progress_from_context function (inlined into single call site).

Tests (31 total, +26 new):
Unit tests for is_valid_git_oid, is_valid_git_oid_or_abbrev, is_null_oid, parse_hook_stdin, parse_reference_transaction_stdin, parse_whitespace_fields, is_path_inside_component, is_path_inside_any_git_ai_dir, hook_has_no_managed_behavior (3 tests), CherryPickBatchState serde roundtrip, RepoHookState serde roundtrip, ForwardMode::None serialization, ensure_hook_symlink idempotency, rebase hook mask double-enable safety, hook name subset invariants, and non-managed hook forwarding behavior (4 tests: provisioned-only-when-original-exists, resync cleanup, no-forward-target skips non-managed, forward-target with empty dir).

Review & Testing Checklist for Human

Verify selective non-managed hook forwarding (CRITICAL): Non-managed hooks (commit-msg, pre-merge-commit, etc.) now get symlinks to the git-ai binary only when the corresponding script exists in the forward target directory (e.g., .husky/commit-msg). Test with Husky/lefthook: (1) verify hooks execute correctly, (2) verify $0 and dirname "$0" resolve to the original hook path (not .git/ai/hooks), (3) add a new hook to the forward dir and run git-ai git-hooks ensure to verify it gets picked up, (4) remove a hook from the forward dir and verify the symlink is cleaned up on next ensure.
Verify self-healing doesn't miss hook changes: Self-healing only triggers when core.hooksPath changes, not when individual hook files are added/removed in the forward dir. If a user adds a new hook to their hooks dir, it won't be picked up until the next git-ai git-hooks ensure or a self-heal trigger. Verify this is acceptable behavior or if we need to add file-watching.
Test non-managed hook execution path: Verify the full flow: git invokes symlink → git-ai binary → handle_git_hook_invocation → execute_forwarded_hook (forwards to original) → run_managed_hook (no-op for non-managed). Confirm forwarding happens before managed logic and that non-managed hooks execute correctly.
Confirm is_valid_git_oid_or_abbrev threshold: Verify that 7 hex chars is sufficient for git's abbreviated OID format. Git's default core.abbrev is "auto" which can go as low as 4 in small repos. Check if sequencer done files ever contain <7 char abbreviations.
Run integration tests: Only unit tests were run locally (cargo test --lib). Run full test suite including tests/hook_modes.rs and tests/stash_attribution.rs to verify no regressions. Run the Heavy Matrix and 10x Nasty Rebase benchmarks from PR #530 to confirm no performance regression.

Notes

All 31 unit tests pass locally (18 from round 1, 5 from round 2, 8 from round 3)
cargo check and cargo clippy pass with no warnings
Link to Devin run: https://app.devin.ai/sessions/54015db92a4e43dc8c7c08d57ad7e520
Requested by: @svarlamov

**Original Pull Request:** https://github.com/git-ai-project/git-ai/pull/540 **State:** closed **Merged:** Yes --- # Fix bugs, simplify parsing, add unit tests for hook handlers ## Summary Follow-up quality improvements to the hook handlers introduced in PR #530. All changes in `src/commands/git_hook_handlers.rs`: **Bug fixes:** - **Non-managed hooks silently broken in hooks-only mode** *(Round 3)*: When git-ai takes over `core.hooksPath` for a repo, only `MANAGED_GIT_HOOK_NAMES` were installed as symlinks. Non-managed core hooks (e.g. `commit-msg`, `pre-merge-commit`) had no symlinks in the managed hooks directory, so pre-existing repo-level or global hooks for those events silently stopped executing. Now selectively installs symlinks (to the git-ai binary) for non-managed hooks **only when the corresponding script actually exists** in the forward target directory. This avoids unnecessary hook interception while preserving `$0`/`dirname` for Husky-style hooks that resolve relative paths. Stale symlinks are cleaned up when the original hook is removed. - `handle_git_hook_invocation` called `should_forward_repo_state_first(None)` twice per hook invocation (once to check existence, once inside `execute_forwarded_hook`). Now cached into `cached_forward_dir` and threaded through. - `latest_cherry_pick_source_from_sequencer` used `is_valid_git_oid` (requires exactly 40 or 64 hex chars) to validate entries in the sequencer done file, which can contain abbreviated SHAs. Added `is_valid_git_oid_or_abbrev` (≥7 hex chars) and use it there instead. - `maybe_enable_rebase_hook_mask` called `SystemTime::now()` twice — once for `session_id` and once for `created_at_unix_ms` — producing potentially inconsistent timestamps. Now computes once and reuses. - `handle_rebase_post_rewrite_from_stdin`: `.last().unwrap_or_else(|| original_commits[0].clone())` had a dead-code panic fallback (vec is guaranteed non-empty by earlier `is_empty()` guard). Simplified to `.last().unwrap()`. **Simplifications:** - Extracted `sync_non_managed_hook_symlinks` to scan the forward target directory and selectively install/clean up non-managed hook symlinks. Extracted `remove_hook_entry` helper to deduplicate cleanup logic. - Extracted `parse_whitespace_fields(stdin, min_fields)` to consolidate the duplicated `split_whitespace` + filter logic between `parse_hook_stdin` and `parse_reference_transaction_stdin`. - Removed 20-line explicit no-op match arm in `run_managed_hook` (`"commit-msg" | "pre-merge-commit" | ...`) that was already covered by the `_ => 0` wildcard. - Extracted `is_null_oid` helper to replace 5 scattered `chars().all(|c| c == '0')` patterns. - Rewrote `hook_has_no_managed_behavior` to derive from `MANAGED_GIT_HOOK_NAMES` constant instead of manually maintaining a 20-entry match list. Self-maintaining and can't drift. - Shared `git_dir` lookup in `hook_requires_managed_repo_lookup` reference-transaction arm (was calling `git_dir_from_context()` twice via separate helper functions). - Removed dead `is_cherry_pick_in_progress_from_context` function (inlined into single call site). **Tests (31 total, +26 new):** Unit tests for `is_valid_git_oid`, `is_valid_git_oid_or_abbrev`, `is_null_oid`, `parse_hook_stdin`, `parse_reference_transaction_stdin`, `parse_whitespace_fields`, `is_path_inside_component`, `is_path_inside_any_git_ai_dir`, `hook_has_no_managed_behavior` (3 tests), `CherryPickBatchState` serde roundtrip, `RepoHookState` serde roundtrip, `ForwardMode::None` serialization, `ensure_hook_symlink` idempotency, rebase hook mask double-enable safety, hook name subset invariants, and non-managed hook forwarding behavior (4 tests: provisioned-only-when-original-exists, resync cleanup, no-forward-target skips non-managed, forward-target with empty dir). ## Review & Testing Checklist for Human - [ ] **Verify selective non-managed hook forwarding** *(CRITICAL)*: Non-managed hooks (commit-msg, pre-merge-commit, etc.) now get symlinks to the git-ai binary **only when the corresponding script exists** in the forward target directory (e.g., `.husky/commit-msg`). Test with Husky/lefthook: (1) verify hooks execute correctly, (2) verify `$0` and `dirname "$0"` resolve to the original hook path (not `.git/ai/hooks`), (3) add a new hook to the forward dir and run `git-ai git-hooks ensure` to verify it gets picked up, (4) remove a hook from the forward dir and verify the symlink is cleaned up on next `ensure`. - [ ] **Verify self-healing doesn't miss hook changes**: Self-healing only triggers when `core.hooksPath` changes, not when individual hook files are added/removed in the forward dir. If a user adds a new hook to their hooks dir, it won't be picked up until the next `git-ai git-hooks ensure` or a self-heal trigger. Verify this is acceptable behavior or if we need to add file-watching. - [ ] **Test non-managed hook execution path**: Verify the full flow: git invokes symlink → git-ai binary → `handle_git_hook_invocation` → `execute_forwarded_hook` (forwards to original) → `run_managed_hook` (no-op for non-managed). Confirm forwarding happens before managed logic and that non-managed hooks execute correctly. - [ ] **Confirm `is_valid_git_oid_or_abbrev` threshold**: Verify that 7 hex chars is sufficient for git's abbreviated OID format. Git's default `core.abbrev` is "auto" which can go as low as 4 in small repos. Check if sequencer done files ever contain <7 char abbreviations. - [ ] **Run integration tests**: Only unit tests were run locally (`cargo test --lib`). Run full test suite including `tests/hook_modes.rs` and `tests/stash_attribution.rs` to verify no regressions. Run the Heavy Matrix and 10x Nasty Rebase benchmarks from PR #530 to confirm no performance regression. ### Notes - All 31 unit tests pass locally (18 from round 1, 5 from round 2, 8 from round 3) - `cargo check` and `cargo clippy` pass with no warnings - Link to Devin run: https://app.devin.ai/sessions/54015db92a4e43dc8c7c08d57ad7e520 - Requested by: @svarlamov  --- <a href="https://app.devin.ai/review/git-ai-project/git-ai/pull/540" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a>