Skip to content

feat: hook ownership model with external manager detection#12980

Open
ichoosetoaccept wants to merge 1 commit intogitbutlerapp:masterfrom
ichoosetoaccept:hook-coexistence-mvp
Open

feat: hook ownership model with external manager detection#12980
ichoosetoaccept wants to merge 1 commit intogitbutlerapp:masterfrom
ichoosetoaccept:hook-coexistence-mvp

Conversation

@ichoosetoaccept
Copy link
Copy Markdown

@ichoosetoaccept ichoosetoaccept commented Mar 22, 2026

Closes #12748

Summary

Adds a hook ownership model so GitButler can coexist with external hook managers (now: prek, later: pre-commit, lefthook, husky, etc.) instead of unconditionally overwriting their hooks.

This PR ships the plumbing + but CLI milestone described in #12748 (comment). It does not include but doctor, Desktop UI integration, or onboarding changes — those are separate follow-ups.

What changed

New but-hooks crate

All hook detection and management logic moved into a dedicated but-hooks crate, following the gitbutler-but- migration. Existing consumers in gitbutler-repo continue working via re-exports.

  • Hook manager detection — content-based signature matching (inspects hook file contents) with a config+binary fallback for managers that are configured but haven't installed hooks yet. Currently supports prek; adding new managers requires a single enum variant + match arms for owns_hook, has_config, is_available, and integration_instructions.
  • Managed hook installation — installs GitButler's guard hooks with ownership checks. Non-GitButler hooks are preserved (not overwritten) unless --force-hooks is used, in which case the original is backed up to <hook>-user.
  • Project-local binary detection — checks .venv/bin/ and node_modules/.bin/ in addition to system PATH, so project-local hook manager installations are detected.

but hook subcommands

Composable CLI entry points for external hook managers to invoke:

  • but hook pre-commit — blocks git commit on gitbutler/workspace with a helpful error directing the user to but commit.
  • but hook post-checkout — prints an informational message when leaving the workspace branch.
  • but hook pre-push — blocks git push on gitbutler/workspace with a helpful error directing the user to but push.
  • but hook status — diagnostic command showing hook mode (managed/external/disabled/unconfigured), per-hook ownership, orphaned hook detection, and context-aware recommendations. Supports human, shell, and JSON output.

Updated lifecycle commands

  • but setup — detects external hook managers before installing. If detected: persists opt-out config, cleans up any previously-installed GitButler hooks, and prints integration instructions. --no-hooks / --force-hooks flags provide manual overrides.
  • but teardown — respects hook ownership during cleanup. Externally-managed hooks are left untouched.

Desktop backend (gitbutler-branch-actions)

Updated the existing update_workspace_commit call site to use ensure_managed_hooks() instead of the old install_managed_hooks_gix(). This is a backend-only change — no UI, no Tauri, no onboarding — ensuring the Desktop app's workspace migration path also respects external hook managers rather than overwriting them. Added a test confirming hooks are not installed when the config opt-out is active.

Automated tests

115 non-ignored tests across but-hooks, but, and gitbutler-branch-actions, organized in five layers:

  • Unit tests (~20) — hook manager detection algorithms (content signatures, config+binary fallback, project-local binary paths in .venv/bin/ and node_modules/.bin/), configuration persistence, backup status formatting
  • Integration tests (~35) — filesystem operations with temp dirs: hook installation, uninstallation, backup/restore roundtrips, staleness detection, force-install overwrites, partial success with warnings, idempotency
  • Shell script execution tests (8) — actually run the installed hook scripts in real git repos: workspace branch guards block git commit, post-checkout cleanup fires when leaving workspace, user hook delegation works on non-workspace branches
  • CLI snapshot tests (~43) — snapbox-based output validation for but hook status, but setup, and but teardown across all flag combinations (--no-hooks, --force-hooks, --json) and hook ownership modes (managed, external, disabled, unconfigured)
  • End-to-end prek integration tests (9, #[ignore]-gated) — real prek binary scenarios; require prek on PATH, run with cargo test -p but -- --ignored hook_prek

Testing patterns: temp dir isolation for every test, environment variable mocking for binary availability (test-overrides feature flag), #[cfg(unix)] guards for permission-specific tests.

Line count breakdown (from uv run prstats.py):

    Tests                +4435   -742    (net +3693)
    Implementation       +2751   -441    (net +2310)
    Other                +84     -6      (net +78)
    ────────────────────────────────────────────────
    Total                +7270   -1189   (net +6081)

Manual testing

In addition to the automated suite above, I manually tested each user-facing scenario end-to-end. I built a release binary (cargo build --release -p but) and ran each scenario against a clean disposable git repo with a local bare remote. I captured screenshots by piping command output through cleen — a tool that renders ANSI terminal output to a PNG — using a pseudo-TTY wrapper (script -q /dev/null) so the binary emits colors through the pipe. The capture script is a local test artifact and is not included in the PR. All on macOS. Twelve scenarios covered:

Happy path

1. but setup on a clean repo — hooks installed

I ran but setup on a fresh repo. It switches to gitbutler/workspace and confirms each hook is in place — one line per hook. Verified that the installed hooks block git commit on the workspace branch with a GITBUTLER_ERROR as expected (pre-existing behavior, not screenshotted).

01_setup

2. but hook status — verify hooks are in place

After setup, I ran but hook status. It shows all three hooks listed as GitButler-managed with green checkmarks. Running with --json emits structured per-hook status including ownership and any warnings (not shown here).

02_hook_status

3. but teardown — clean exit

I ran but teardown. It reports each removed hook individually before checking out the target branch. Only GitButler-managed hooks are removed — any hooks not owned by GitButler are left untouched. The managed post-checkout script also auto-removes hooks on a plain git checkout <branch> away from the workspace.

04_teardown

External manager coexistence

4. prek detected — setup defers to external manager

With prek.toml present and prek install already run, I ran but setup. It detects prek, leaves the hooks untouched, persists gitbutler.installHooks=false, and prints ready-to-paste integration instructions.

05_setup_prek_detected

5. but hook status — external ownership shown

I then ran but hook status. It shows external manager mode with each hook owned by prek and actionable recommendations for hooks not yet wired up.

06_hook_status_prek

6. but teardown — prek hooks left untouched

I ran but teardown in the prek-managed repo. It left prek's hooks untouched and printed guidance about removing but hook entries from the prek config if you want to fully disable the workspace guards.

07_teardown_prek

7. prek detected — config+binary fallback (before prek install)

With only prek.toml present and the binary on PATH (no hook files yet), I ran but setup. It still detects prek via the config+binary fallback and shows the same integration instructions.

08_setup_config_fallback

Override behaviors and re-run

8. --no-hooks flag — persistent opt-out

I ran but setup --no-hooks. It skips hook installation and persists gitbutler.installHooks=false.

09_setup_no_hooks

9. Re-run detection — config opt-out preserved

After opting out (scenario 8), I ran but setup again. It shows the config-preserved opt-out without re-scanning and hints at --force-hooks to re-enable.

10_setup_rerun

10. --force-hooks — override external manager detection

In a repo where prek was previously detected (installHooks=false), I ran but setup --force-hooks. It skips detection entirely, backs prek's hooks up to <hook>-user, installs GitButler's managed hooks in their place, and reports each one with ✓ Installed pre-commit etc.

11_setup_force_hooks

11. but teardown with user backup — hooks restored

Continuing from scenario 10, I ran but teardown. It reports each hook removal and shows Restored pre-commit (your original hook is back) for the hook that was backed up to pre-commit-user.

12_teardown_restore

Edge case

12. Pre-existing plain user hook — partial install with actionable warning

I placed a plain pre-commit hook (not owned by any known manager) and ran but setup. It installs post-checkout and pre-push (each with ✓ Installed …) and skips pre-commit with a yellow warning including the --force-hooks override hint.

13_setup_user_hook

Architecture

but setup / Desktop app
    │
    ▼
ensure_managed_hooks()          ← single entry point for all callers
    ├── skip if: installHooks=false  OR  external manager detected
    │                                     ├── signature match in hook files
    │                                     └── config+binary fallback (no files yet)
    └── install_managed_hooks() ← only when both guards pass
            └── install_hook()  ← per-hook, with ownership check

External hook manager (prek, etc.)
    │
    ▼
but hook pre-commit / post-checkout / pre-push
    └── workspace branch check → block or allow

Notes

gix::discover_with_environment_overrides() in hook subcommands

Byron flagged in his initial review that bare gix::discover() is not the right call for hook subcommands — Git sets GIT_DIR / GIT_WORK_TREE before invoking hooks and gitoxide has gix::open_with_environment_overrides() for exactly this. We use gix::discover_with_environment_overrides() which satisfies both requirements: it honours the git-provided environment variables when present (so git commit invocations work correctly), and falls back to a directory walk when invoked directly by an external manager like prek without GIT_DIR set. agents.md says not to re-discover repositories inside commands; these subcommands are a documented exception since they run as standalone hook subprocesses with no Context.

Skill files updated

crates/but/skill/references/reference.md was updated with the new but hook subcommands and the --no-hooks/--force-hooks flags for but setup, per the agents.md convention.

Scope note

This PR is scoped to plumbing + CLI only, per the agreed plan in #12748:

In scope Out of scope (follow-up)
but-hooks crate but doctor / but-diagnose
but hook subcommands Desktop UI / settings
but setup / but teardown Onboarding flow
Backend call site update Additional hook manager support

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 22, 2026

Someone is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

@ichoosetoaccept ichoosetoaccept force-pushed the hook-coexistence-mvp branch 2 times, most recently from bf4324d to 519dbd7 Compare March 22, 2026 22:16
@Byron
Copy link
Copy Markdown
Collaborator

Byron commented Mar 23, 2026

Thanks a lot for resubmitting the PR, I will take it on!

I appreciate the 'relatively' concise PR description with notes to help organising the review.
Regarding the validation checklist: it does not instill trust when I see any item that is checked by CI. Also, agents tend to say "I ran this and that for validation" so that alone makes it feel generated. That validation checklist is exactly what shouldn't be generated, it's to me what a human does to validate the generated code, which helps the reviewer to know to what extend the generated code was scrutinized.

Regarding gix::discover() in hook subcommands, I don't think this is how hooks should work. Git does provide environment variables to anchor the hook which also invokes git naturally. gitoxide has another way: gix::open_with_environment_overrides() which should be used here.

Please also put all integration tests, i.e. tests for the public API, into tests/ and follow the structure of existing but-* crates. If there must be unit-tests, i.e. tests of private functions, please factor them into their own module files. Please make sure tests look structured when I run cargo test -p \"$ZED_CUSTOM_RUST_PACKAGE\" -- --list | sed 's/: test$//' | sed 's/::/\\//g' | tree --fromfile, so do not use test_ prefixes for test functions and let integration tests follow the structure of the functions they are testing.

Please address the above before I go into a deeper review.
We might iterate a couple of times as I plan to hand bigger refactors off to you, while doing smaller refactors and changes myself.

PS: Please ping me in a message when you think everything is addressed.

@ichoosetoaccept
Copy link
Copy Markdown
Author

ichoosetoaccept commented Mar 23, 2026

You're very welcome!

Thank you for the detailed feedback. The PR is still a draft so I appreciate your pointers. I'll start iterating until this gets closer to being properly reviewable (within the confines of being unreviewably large 🙂).

@Byron
Copy link
Copy Markdown
Collaborator

Byron commented Mar 25, 2026

My hope is that this PR is at least half-tests. Do you have data on that?

@ichoosetoaccept
Copy link
Copy Markdown
Author

ichoosetoaccept commented Mar 26, 2026

My hope is that this PR is at least half-tests. Do you have data on that?

Hey @Byron . Indeed I do. In fact, most of the code is tests!

The result of running this script from root of the project...

#!/usr/bin/env python3
# /// script
# requires-python = ">=3.10"
# ///
"""Break down PR diff stats by category: tests, implementation, other."""

import subprocess
import sys


CATEGORIES = [
    ("Tests", ["*/tests/*"]),
    ("Implementation", [":!*/tests/*", "*.rs"]),
    ("Other", [":!*.rs"]),
]


def numstat(base: str, pathspecs: list[str]) -> tuple[int, int]:
    result = subprocess.run(
        ["git", "diff", "--numstat", f"{base}..HEAD", "--", *pathspecs],
        capture_output=True,
        text=True,
    )
    added = deleted = 0
    for line in result.stdout.strip().splitlines():
        parts = line.split("\t")
        if parts[0] != "-":  # skip binary files
            added += int(parts[0])
            deleted += int(parts[1])
    return added, deleted


def main() -> None:
    if len(sys.argv) > 1:
        base = sys.argv[1]
    else:
        # Auto-detect merge base with master
        result = subprocess.run(
            ["git", "merge-base", "HEAD", "master"],
            capture_output=True,
            text=True,
        )
        base = result.stdout.strip()
        if not base:
            print("Could not determine merge base. Pass a base ref explicitly.", file=sys.stderr)
            sys.exit(1)

    print(f"Base: {base[:10]}\n")
    total_add = total_del = 0
    for label, specs in CATEGORIES:
        added, deleted = numstat(base, specs)
        total_add += added
        total_del += deleted
        print(f"  {label:<20} +{added:<6} -{deleted:<6} (net {added - deleted:+d})")

    print(f"  {'─' * 48}")
    print(f"  {'Total':<20} +{total_add:<6} -{total_del:<6} (net {total_add - total_del:+d})")


if __name__ == "__main__":
    main()

is

uv run prstats.py
Base: 8be2425f3a

  Tests                +3715   -631    (net +3084)
  Implementation       +2203   -451    (net +1752)
  Other                +73     -4      (net +69)
  ────────────────────────────────────────────────
  Total                +5991   -1086   (net +4905)

Testing is the reason I've been taking so long. I discovered some small issues when testing myself and am just about to push fixes, but in general, I tried to cover everything that is meaningfully testable by test code with tests.

@ichoosetoaccept
Copy link
Copy Markdown
Author

ichoosetoaccept commented Mar 26, 2026

BTW in an effort to do the most I can to make your review a "nicer" experience, I sync my fork's origin/master with upstream/master multiple times a day and rebase onto the latest changes. I don't remember whether I ever had a merge conflict. I'm inclined to say that's good news 🙂.

EDIT: Of course I ran into conflicts JUST after posting this. Handling it.

@ichoosetoaccept
Copy link
Copy Markdown
Author

Update: discovered a few issues when testing this manually, myself, for real. Implementing multiple usability and clarity fixes. "Draft" status is still appropriate.

@ichoosetoaccept ichoosetoaccept force-pushed the hook-coexistence-mvp branch 11 times, most recently from dadc0f5 to 0bdc9f6 Compare March 29, 2026 15:15
Implement two-mode hook ownership: GitButler-managed (installs hooks
directly) vs externally-managed (defers to prek/lefthook/husky).

- Add hook_manager.rs: detect external managers via config + binary gate
- Refactor managed_hooks.rs: ensure_managed_hooks() shared entry point
- Add pre-push guard hook (blocks pushing gitbutler/workspace)
- Add --no-hooks / --force-hooks escape hatches to setup/teardown
- Hook lifecycle: staleness detection, auto-update, config persistence
- Desktop app (integration.rs) uses same ensure_managed_hooks() path

Addresses gitbutlerapp#12748

refactor: extract but-hooks crate from gitbutler-repo

feat: add but hook CLI subcommands for hook manager integration

Expose GitButler workspace guard and cleanup as standalone CLI commands
that external hook managers can invoke from their configuration:

- but hook pre-commit: blocks git commit on gitbutler/workspace
- but hook post-checkout: info message when leaving workspace branch
- but hook pre-push: blocks git push on gitbutler/workspace
- but hook status: diagnostic view of hook ownership and integration state

Follows up on hook-ownership-core.
@ichoosetoaccept
Copy link
Copy Markdown
Author

ichoosetoaccept commented Mar 29, 2026

Hey @Byron
I think it's ready for review. I updated the PR description with the latest details. I performed all the manual tests myself and wrote out the descriptions for the scenarios, then had AI help me transform my mess of notes into a coherent story to hopefully make it logical to follow from a reviewer's POV. The screenshots are meant to help illustrate what these changes look like in practice.

I have lots of ideas for followups and improvements but had to draw the line somewhere. I apologize this got as large as it did but I hope you'll appreciate that more than half of the contribution is test code.

Consider yourself pinged and do let me know if anything substantial needs to change or if you'd prefer a different direction altogether.

Cheers!

@ichoosetoaccept ichoosetoaccept marked this pull request as ready for review March 29, 2026 18:15
@Byron
Copy link
Copy Markdown
Collaborator

Byron commented Apr 1, 2026

Thanks so much for staying on top of this, but as I am currently putting much less hours in that I have to and other priorities came up, it will be a while until this PR can get a proper review. I will, however, see if quick glimpses can already provide some meaningful feedback.
Lastly, maybe it's a good thing you can test but with the PR applied to find more real-world issues in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

but setup displaces hooks owned by hook managers (prek, pre-commit, husky) without detecting them, and provides no escape hatch

2 participants