Add dual-mode git-ai operation via global hooks and wrapper#530
Add dual-mode git-ai operation via global hooks and wrapper#530
Conversation
Add hook-name binary dispatch and managed global hooks installation with persisted forward state for previous global/local hooksPath. Add repo-level hook self-heal from checkpoint flow, recursion suppression for internal git subprocesses, and wrapper+hooks coexistence safeguards to avoid double execution. Extend test harness with wrapper/hooks/both modes and add hook mode regression coverage.
| } | ||
|
|
||
| let local_config_path = repo.path().join("config"); | ||
| let current_local_hooks = | ||
| read_hooks_path_from_config(&local_config_path, gix_config::Source::Local); | ||
|
|
||
| // If no repo-level hooksPath is configured, do nothing. | ||
| let Some(current_local_hooks) = current_local_hooks else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| if current_local_hooks.trim().is_empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let repo_state = repo_state_path(repo); | ||
| let current_is_managed = is_managed_hooks_path_str(current_local_hooks.trim()); | ||
|
|
||
| if !current_is_managed { | ||
| save_hook_state( | ||
| &repo_state, | ||
| &HooksPathState { | ||
| previous_hooks_path: current_local_hooks.trim().to_string(), | ||
| }, |
There was a problem hiding this comment.
🚩 execute_forwarded_hook captures stdout/stderr which changes timing behavior
The forwarded hook process has its stdout/stderr piped (Stdio::piped()) at lines 557-558, and the output is replayed after the process completes (lines 574-575). This means the forwarded hook's output is buffered entirely in memory and written after completion, rather than streaming in real-time. For hooks that produce progress output or interactive prompts, this could cause user-visible behavior changes compared to pre-installation behavior where git ran user hooks directly with inherited stdio. This is a design trade-off for clean output separation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if is_post_commit_amend(&repo) { | ||
| // For --amend, post-rewrite (amend) owns rewrite mapping. | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🚩 Hooks mode skips commit_post_command_hook for amend — potential attribution loss
In the post-commit managed hook handler, amend commits are detected and skipped (line 1657–1660), with a comment that post-rewrite (amend) owns the rewrite mapping. However, in the wrapper path (git_handlers.rs:399-408), commit_post_command_hook runs unconditionally for ALL commits including amends — this is the function that processes the working log and creates fresh authorship notes from the current working state.
The post-rewrite amend handler (lines 1697–1711) only remaps the OLD commit's authorship note to the new commit via handle_rewrite_log_event(commit_amend(...)). If a user checkpoints new AI code and then runs git commit --amend, the working log has the new attributions, but in hooks mode only the old note is remapped — the new working log data may not be incorporated.
The test hooks_mode_amend_uses_single_amend_rewrite_event validates event counts in the rewrite log but does not assert on authorship attribution correctness after amend. This may be an intentional trade-off for the initial hooks mode implementation, but it represents a behavioral divergence from wrapper mode that could lose AI attributions during amend.
Was this helpful? React with 👍 or 👎 to provide feedback.
| thread_local! { | ||
| static INTERNAL_GIT_HOOKS_DISABLED_DEPTH: Cell<usize> = const { Cell::new(0) }; | ||
| } | ||
|
|
||
| pub struct InternalGitHooksGuard; | ||
|
|
||
| impl Drop for InternalGitHooksGuard { | ||
| fn drop(&mut self) { | ||
| INTERNAL_GIT_HOOKS_DISABLED_DEPTH.with(|depth| { | ||
| let current = depth.get(); | ||
| if current > 0 { | ||
| depth.set(current - 1); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /// Disable managed git hooks for internal `git` subprocesses executed through `exec_git*`. | ||
| /// Use this guard around higher-level operations that already execute hook logic explicitly. | ||
| pub fn disable_internal_git_hooks() -> InternalGitHooksGuard { | ||
| INTERNAL_GIT_HOOKS_DISABLED_DEPTH.with(|depth| depth.set(depth.get() + 1)); | ||
| InternalGitHooksGuard | ||
| } | ||
|
|
||
| fn should_disable_internal_git_hooks() -> bool { | ||
| INTERNAL_GIT_HOOKS_DISABLED_DEPTH.with(|depth| depth.get() > 0) | ||
| } |
There was a problem hiding this comment.
🚩 disable_internal_git_hooks guard is thread-local — background threads spawned inside hooks won't inherit it
The INTERNAL_GIT_HOOKS_DISABLED_DEPTH counter at src/git/repository.rs:26 is thread_local!. The guard created in run_pre_command_hooks (src/commands/git_handlers.rs:322) and run_post_command_hooks (src/commands/git_handlers.rs:399) only suppresses hooks on the current thread. If any hook logic spawns a background thread that calls exec_git, that thread won't have the guard active and its git subprocesses will run with hooks enabled. Currently, the push hooks (push_hooks::push_pre_command_hook) spawn a background thread to push authorship notes. Those threads call push_authorship_notes which uses exec_git. Since the push happens via exec_git on a different thread, hooks would NOT be suppressed for those internal git calls. In practice this is likely fine because the push operations (git push) are not hook-sensitive commands, but it's a design subtlety worth documenting.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Validation