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.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| 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.
| "pre-push" => { | ||
| let parsed = parsed_invocation("push", hook_args.to_vec()); | ||
| push_hooks::run_pre_push_hook_managed(&parsed, &repo); | ||
| 0 |
There was a problem hiding this comment.
🟡 Hook-mode pre-push cannot detect --dry-run, --delete, or --mirror flags from original push command
In hook mode, the pre-push handler at src/commands/git_hook_handlers.rs:1786 constructs a ParsedGitInvocation using the hook's own arguments (<remote-name> <url>), not the original git push command-line flags. The should_skip_authorship_push check then never sees --dry-run, --delete, or --mirror.
Root Cause
The git pre-push hook receives only <remote-name> and <url> as arguments — it does NOT receive the original push flags. In wrapper mode (src/commands/hooks/push_hooks.rs:8-17), push_pre_command_hook has access to the real parsed_args.command_args which contain the original flags like --dry-run. But in hook mode:
// src/commands/git_hook_handlers.rs:1785-1788
"pre-push" => {
let parsed = parsed_invocation("push", hook_args.to_vec()); // hook_args = ["origin", "https://..."]
push_hooks::run_pre_push_hook_managed(&parsed, &repo);
0
}run_pre_push_hook_managed calls should_skip_authorship_push(&parsed_args.command_args) at src/commands/hooks/push_hooks.rs:57, which checks for --dry-run, --delete, and --mirror — none of which exist in the hook args.
Impact: When a user runs git push --dry-run, hook mode will still push authorship notes to the remote (a real side-effect during what was supposed to be a no-op). Similarly for --delete and --mirror operations where authorship push should be skipped.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged. This remains intentionally deferred in this changeset per approved scope (Workstreams 1/3/4 only). We did not modify hook-mode pre-push semantics in this patch, so this thread stays open for dedicated follow-up.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Follow-up implemented for Workstreams 1/3/4 only.
Deferred intentionally (out of scope this iteration):
|
Bug fixes: - Cache forward hooks dir lookup in handle_git_hook_invocation to avoid redundant state file reads (was reading twice per hook invocation) - Add is_valid_git_oid_or_abbrev for sequencer done file parsing which may contain abbreviated commit hashes (>= 7 chars) Simplifications: - Extract parse_whitespace_fields to consolidate parse_hook_stdin and parse_reference_transaction_stdin which shared identical parsing logic - Remove 19-line redundant no-op hook match arm in run_managed_hook that duplicated hook_has_no_managed_behavior; wildcard already handles this Tests (18 new unit tests): - is_valid_git_oid: SHA-1, SHA-256, rejects short/invalid - is_valid_git_oid_or_abbrev: abbreviated hex acceptance - parse_reference_transaction_stdin: 3-field extraction, incomplete lines - parse_hook_stdin: single-field skip, extra field handling - parse_whitespace_fields: empty input edge cases - is_path_inside_component: nested segment detection - is_path_inside_any_git_ai_dir: .git/ai subtree detection - hook_has_no_managed_behavior: correct classification - cherry_pick_batch_state: serialization roundtrip - repo_hook_state: serialization roundtrip - forward_mode_none: serialization format - ensure_hook_symlink: idempotency - rebase_hook_mask: double-enable is noop - managed/rebase hook name subset invariants Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
…anaged_behavior, share git_dir lookup, remove dead code, add tests Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
…, fix silent hook breakage in hooks-only mode Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
… original exists in forward dir - Extract sync_non_managed_hook_symlinks to scan forward target dir and only install symlinks for non-managed hooks whose original script exists there - Symlinks point to git-ai binary (not original script) so forwarding preserves $0/dirname for Husky-style hooks that resolve relative paths - Stale symlinks cleaned up when original hook is removed from forward dir - Self-healing via ensure_repo_hooks_installed re-scans and keeps in sync - Extract remove_hook_entry helper to deduplicate cleanup logic - 4 new tests: provisioned-only-when-original-exists, resync cleanup, no-forward-target skips non-managed, forward-target with empty dir Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
Co-Authored-By: Sasha Varlamov <sasha@sashavarlamov.com>
…dlers-quality Improve hook handler quality: fix bugs, simplify, add tests
|
|
2 participants
Summary
.git/ai/hooks) with persisted repo state in.git/ai/git_hooks_state.json-c core.hooksPath=<forward-or-null>) so child git does not execute git-ai managed hooks.git/aiand.git-aipathsgit-ai git-hooks ensureper copied run repo and asserting local hooksPath/hook surfacePerformance Results
Heavy Matrix (baseline =
current_wrapper, margin target = <=25%)Run:
benchmark_modes_vs_main.py --iterations-basic 5 --iterations-complex 4 --margin-pct 25 --margin-baseline current_wrapper --enforce-marginResult: 15/16 checks passing (only
cherry_pick_threein hooks mode misses).10x Nasty Rebase Matrix (cpPython workload)
Run:
benchmark_nasty_modes_vs_main.py --feature-commits 900 --main-commits 350 --side-commits 250 --files 6 --lines-per-file 1500 --burst-every 15 --repetitions 1 --margin-pct 25 --margin-baseline current_wrapper --enforce-marginResult: 6/6 checks passing.
10x Cherry-Pick Stress (targeted)
Workload: 30 sequential cherry-picks (
3reps/mode), with upstream divergence.Validation
cargo check -qpython3 scripts/benchmarks/git/benchmark_modes_vs_main.py --iterations-basic 5 --iterations-complex 4 --margin-pct 25 --margin-baseline current_wrapper --enforce-marginpython3 scripts/benchmarks/git/benchmark_nasty_modes_vs_main.py --feature-commits 900 --main-commits 350 --side-commits 250 --files 6 --lines-per-file 1500 --burst-every 15 --repetitions 1 --margin-pct 25 --margin-baseline current_wrapper --enforce-margin/var/folders/6w/760cbpln2cs16fxprwshynb80000gn/T/git-ai-cherrypick-scale-ayt78i0x/summary_cherrypick30.json