feat: hook ownership model with external manager detection#1
Open
ichoosetoaccept wants to merge 730 commits into
Open
feat: hook ownership model with external manager detection#1ichoosetoaccept wants to merge 730 commits into
ichoosetoaccept wants to merge 730 commits into
Conversation
ichoosetoaccept
added a commit
that referenced
this pull request
Apr 21, 2026
Address Greptile review on #1: - hook.rs shell output now escapes embedded apostrophes in hooks_path, external_manager, and per-hook owner labels using the POSIX '\'' idiom, so a Unix path with an apostrophe can't break out of the single-quoted literal when callers source the output. - orphaned_hooks_remove_command now takes &Path so the suggested `rm ...` points at the real default hooks dir (respecting linked worktrees / GIT_DIR overrides) instead of hardcoding .git/hooks/, which previously gave advice contradicting the warning above it. - Snapshot tests updated with [..] wildcards for the now-absolute paths.
Member
Author
|
This change is part of the following stack: Change managed by git-spice. |
ichoosetoaccept
added a commit
that referenced
this pull request
Apr 22, 2026
Address Greptile review on #1: - hook.rs shell output now escapes embedded apostrophes in hooks_path, external_manager, and per-hook owner labels using the POSIX '\'' idiom, so a Unix path with an apostrophe can't break out of the single-quoted literal when callers source the output. - orphaned_hooks_remove_command now takes &Path so the suggested `rm ...` points at the real default hooks dir (respecting linked worktrees / GIT_DIR overrides) instead of hardcoding .git/hooks/, which previously gave advice contradicting the warning above it. - Snapshot tests updated with [..] wildcards for the now-absolute paths.
b1874c0 to
4b898dd
Compare
…ranch-reword-strict-name-validation fix(but): make name validation in `but reword <branch>` strict
We we're missing one line of context when scrolling to a hunk.
…go-flamegraph build: pull in working version of cargo-flamegraph
…s-off-by-one fix(tui): fix full screen details off-by-one
…rget-outside-workspace fix problem with `commit already belongs to another branch`
Add full-screen size for modal
…odal UI Modal: Add new size for full-screen modals
The performance of `but branch list` without any extra options (a.k.a `but branch`) is very poor on repositories with many branches. The culprit is the "empty branch" computation where we check if the branch is ahead of the workspace or not. This computation was performed eagerly, and as the defaults include remote branches, repositories like https://github.com/microsoft/vscode with its ~4k branches made `but branch` take on the order of minutes to complete. That's even on powerful workstations. This commit makes the computation lazy, which solves the problem for the average case. The X latest branches that satisfy the criteria of "non-empty" will always be found within the X+Y first scanned branches. In a typical repository, I've found that Y is somewhat proportional to X. For X=20, Y tends to be negligible. Hence, we solve the average case with lazy computation, while the worst case remains the same. You can still trigger worst case performance by running with the `--all` flag, but at least then you've made an explicit choice about it. `but branch list` requires a more fundamental rewrite to address larger UX concerns, which is why this change targets this performance issue in isolation. Performance before: * GitButler repo: ~500ms * VS Code repo: ~1m10s Performance after: * GitButler repo: ~60ms * VS Code repo: ~160ms
Clicking twice results in a `.type` error.
…for-login Make login buttons async to prevent double click
This broke the overlay gradient. Oops.
…zytn Lite: disallow deleting top branch reference inside stack of multiple branches
Determine the relevant commits in the divergence between a branch and its remote. Base on that and a given integration strategy, generate a set of steps for integrating those changes into the local branch. Create a function that executes those integration steps in memory for preview or actual realization.
…slop Lite: diff sticky headers + teach commit shortcuts
…am-integration graphy remote branch integration
feat(cli): beginnings of cli revamp (commit2/move2/squash2)
…korx Lite: generic navigation index + refine files list and selection types
…mltw Lite: misc + (delete commit: shortcut + preserve selection)
…ction-with-transaction-with-perm feat: add `but_transaction::with_transaction_with_perm`
End state of the forge refactor: the desktop renderer no longer
carries a per-forge class hierarchy. Components inject forge-agnostic
services directly, every forge-aware decision lives in Rust, and the
Octokit / gitbeaker clients are gone.
Each forge (GitHub, GitLab, Bitbucket, Azure) used to have its own
class behind a `Forge` interface, with ~24 components branching on
forge type. Once forge calls moved to Rust (via Tauri commands) those
classes became thin wrappers around identical backend calls; this
removes them.
## Rust
- `forge_info(projectId)` -> ForgeInfo { name, baseUrl, commitUrlPath,
prUrlPath, unit, posthogLabel, capabilities }. One round trip gives
the renderer everything it needs to build URLs, label PR/MR text,
and gate capability-specific UI without forge-name branching.
- `forge_compare_branch_url(projectId, base, branch, fork)` builds the
per-forge compare URL (GitHub / GitLab / Bitbucket / Azure).
- `ReviewMergeStatus.is_mergeable` (GitHub clean/unstable/has_hooks,
GitLab can_be_merged) and a forge-agnostic `get_repo_info(projectId)`
keep forge state-string knowledge out of the renderer.
- `publish_review` keeps the GitHub fork `head_owner:` prefix in Rust
(the renderer was duplicating it).
- Commands wired into both the Tauri and but-server dispatchers; SDK
regenerated.
## TypeScript
- Five singleton services in `lib/forge/` wired in `bootstrap/deps.ts`,
each behind an InjectionToken: PrService (PR_SERVICE),
ListingService (LISTING_SERVICE), RepoService (REPO_SERVICE),
ChecksMonitor (CHECKS_MONITOR), ForgeInfoService (FORGE_INFO_SERVICE)
— plus a `useForgeAuth(projectId)` hook and pure
`commitUrl` / `prUrl` helpers.
- `PrNumberSync` gates on the `listService` capability instead of a
hardcoded `=== "github"`, so GitLab MRs now associate to branches.
- 24 components migrated to inject services and read `forgeInfo`
instead of `forge.current.*`.
## Deletions
- The five per-forge Forge classes and all four `*Branch` classes.
- `DefaultForgeFactory` + the `DEFAULT_FORGE_FACTORY` token.
- The whole `lib/forge/interface/` set of interfaces.
- Octokit / gitbeaker plumbing: ghQuery, githubClient, headers,
gitlabClient, mockGitHubApi.
- `GitHubApi` / `GitLabApi` RTK Query slices (ClientState now holds
only backendApi) and the `+layout.svelte` setToken/setHost effects.
- `@octokit/rest` and `@gitbeaker/rest` dependencies.
## Tests
- Rust unit tests for the forge URL builders — base, commit, PR, and
compare URLs for all four forges (guards the hyperlink formats
Bitbucket/Azure have no other integration to surface).
- Playwright forge spec (mocked forge responses): CI-checks badge
pass/fail, GitLab MR labelling + checks-capability gating,
Bitbucket/Azure affordance absence, and the merge-button gates
(mergeable, not-mergeable, stacked target, different base repo).
## Verified
- pnpm check clean (2637 files)
- cargo clippy --workspace --all-targets -- -D warnings clean
- cargo fmt --check --all clean
- desktop unit tests + forge e2e green
…checks desktop: collapse forge plumbing into direct Rust calls
Introduce a hook ownership model so GitButler coexists with external
hook managers (prek today; pre-commit/lefthook/husky trivially follow)
instead of unconditionally overwriting their hooks.
- New `but-hooks` crate extracted from `gitbutler-repo`. Detection
uses content-signature matching with a config+binary fallback for
managers that are configured but haven't installed hooks yet.
- `ensure_managed_hooks()` is the shared entry point for both the CLI
(`but setup`) and the Desktop backend (`integration.rs`). It skips
installation when `gitbutler.installHooks=false` or an external
manager is detected.
- New universal subcommands `but hook install [--force]` and
`but hook uninstall`, plus the existing runtime guards
`but hook {pre-commit,post-checkout,pre-push,status}`. All work in
modern (non-legacy) builds, so external hook managers can wire them
into their config in any build flavor.
- `but setup` / `but teardown` shrink to thin adapters that delegate
to the universal install/uninstall functions. `--no-hooks` and
`--force-hooks` provide manual overrides; non-GitButler hooks are
backed up to `<hook>-user` when force-installed, and post-checkout
cleanup verifies the active hook still carries the GitButler
signature before restoring a backup.
- Output is routed through the semantic `theme::get()` paint tokens
(success / attention / error / info / hint / command_suggestion)
introduced upstream, replacing direct `colored::Colorize` usage.
- Structured JSON is available on every new subcommand
(`HookInstallReport`, `HookUninstallReport`) and embeds into
`but setup`'s SetupResult without duplicating output.
Addresses gitbutlerapp#12748
4b898dd to
ddc1156
Compare
ichoosetoaccept
pushed a commit
that referenced
this pull request
Jul 8, 2026
The PR-listing toasts ("Failed to list open pull requests" et al.)
are the #1 cluster on 0.20.x. They fire on every polled list_reviews
round-trip while the user is offline, where there's nothing to act on.
list_open_pulls / list_pulls_for_base / list_pulls_for_commit /
get_pull_request now:
- route their status check through a shared ensure_success helper that
returns HttpStatusError, so 401s can be downcast downstream.
- have their result mapped through classify_forge_error, which tags
reqwest transport failures as Code::NetworkError and HTTP 401s as
Code::GitHubTokenExpired.
The desktop's CLASSIFICATIONS table gains NetworkError: silent so the
toast and capture are suppressed entirely. GitHubTokenExpired keeps
its existing re-auth toast hint.
Mutating PR endpoints (create / update / merge / set_*) are left alone
so user-initiated writes still surface failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes gitbutlerapp#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 +
butCLI milestone described in gitbutlerapp#12748 (comment). It does not includebut doctor, Desktop UI integration, or onboarding changes — those are separate follow-ups.After a recent rebase onto upstream master, the PR was restructured so hook lifecycle commands are universal rather than legacy-only — modern (non-legacy) builds can now install and uninstall managed hooks too. Output styling was migrated to upstream's new
theme::get()paint tokens. See "Restructure" below.What changed
New
but-hookscrateAll hook detection and management logic moved into a dedicated
but-hookscrate, following thegitbutler-→but-migration. Existing consumers ingitbutler-repocontinue working via re-exports.owns_hook,has_config,is_available, andintegration_instructions.--force-hooksis used, in which case the original is backed up to<hook>-user..venv/bin/andnode_modules/.bin/in addition to system PATH, so project-local hook manager installations are detected.Universal
but hooksubcommandsComposable CLI entry points that work in both legacy and modern builds — external hook managers, scripts, and modern-mode users can all invoke them:
but hook install [--force]— install GitButler-managed hooks. Detects external managers and refuses to clobber them;--forceoverrides detection and re-enablesgitbutler.installHooks=true. Universal: works without legacy crates.but hook uninstall— best-effort cleanup, signature-checks each hook so user/external hooks are never touched. Restores any pre-existing hook from<hook>-user. Universal.but hook pre-commit— blocksgit commitongitbutler/workspacewith a helpful error directing the user tobut commit.but hook post-checkout— prints an informational message when leaving the workspace branch.but hook pre-push— blocksgit pushongitbutler/workspacewith a helpful error directing the user tobut push.but hook status— diagnostic showing hook mode (managed/external/disabled/unconfigured), per-hook ownership, orphaned hook detection, and context-aware recommendations. Supports human, shell, and JSON output.All emit structured JSON via
--format json(or--json).Updated lifecycle commands (legacy-only)
but setupandbut teardownremain legacy-gated (the commands themselves are#[cfg(feature = "legacy")]), but their hook handling shrinks to thin adapters that delegate tobut hook install/but hook uninstall:but setup— translates--no-hooks(persistinstallHooks=falseand calluninstall) and--force-hooks(translate to--force). Embeds the install report into its existingSetupResultJSON without duplicating output.but teardown— callsuninstalland surfaces the "externally managed" hint when prek (or similar) is detected in the hooks directory.Restructure (post-rebase)
After this branch was rebased onto a fresh upstream master, two upstream landings prompted a restructure to keep the PR aligned with current direction:
theme::get()API with semantic tokens (success,attention,info,hint,command_suggestion,error). All ~40colored::Colorizecall sites in our changes (command/hook.rs,legacy/setup.rs,legacy/teardown.rs) now use the new API.colored::Colorizeis dropped from those files.legacy/setup.rs(244 lines) andlegacy/teardown.rs(~75 lines) into universalcommand::hook::install/uninstallfunctions. The legacy adapters are now ~25 / ~5 lines that delegate. As a side effect, modern (non-legacy) builds gainbut hook install/uninstallfor free.Desktop backend (gitbutler-branch-actions)
Updated the existing
update_workspace_commitcall site to useensure_managed_hooks()instead of the oldinstall_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
125 non-ignored tests across
but-hooks,but, andgitbutler-branch-actions, organized in five layers:.venv/bin/andnode_modules/.bin/), configuration persistence, backup status formatting.git commit, post-checkout cleanup fires when leaving workspace, user hook delegation works on non-workspace branches.snapbox-based output validation forbut setup,but teardown,but hook status, plus the newbut hook install/but hook uninstalldirect-subcommand tests (tests/but/command/hook_install.rs,hook_uninstall.rs). Covers all flag combinations (--no-hooks,--force-hooks,--force,--json) and ownership modes (managed, external, disabled, unconfigured). The new direct-subcommand tests pass in both default (legacy) and--no-default-features(modern) builds.#[ignore]-gated) — realprekbinary scenarios; requireprekon PATH, run withcargo test -p but -- --ignored hook_prek.Testing patterns: temp dir isolation for every test, environment variable mocking for binary availability (
test-overridesfeature flag),#[cfg(unix)]guards for permission-specific tests.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 throughcleen— 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 setupon a clean repo — hooks installedI ran
but setupon a fresh repo. It switches togitbutler/workspaceand confirms each hook is in place — one line per hook. Verified that the installed hooks blockgit commiton the workspace branch with aGITBUTLER_ERRORas expected (pre-existing behavior, not screenshotted).2.
but hook status— verify hooks are in placeAfter setup, I ran
but hook status. It shows all three hooks listed as GitButler-managed with green checkmarks. Running with--jsonemits structured per-hook status including ownership and any warnings (not shown here).3.
but teardown— clean exitI 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 plaingit checkout <branch>away from the workspace.External manager coexistence
4. prek detected — setup defers to external manager
With
prek.tomlpresent andprek installalready run, I ranbut setup. It detects prek, leaves the hooks untouched, persistsgitbutler.installHooks=false, and prints ready-to-paste integration instructions.5.
but hook status— external ownership shownI then ran
but hook status. It shows external manager mode with per-hook ownership — prek owns the slots it has installed, and slots prek hasn't wired up yet appear as✗ not configured (prek)with actionable warnings + recommendations.6.
but teardown— prek hooks left untouchedI ran
but teardownin the prek-managed repo. It left prek's hooks untouched and printed guidance about removingbut hookentries from the prek config if you want to fully disable the workspace guards.7. prek detected — config+binary fallback (before
prek install)With only
prek.tomlpresent and the binary on PATH (no hook files yet), I ranbut setup. It still detects prek via the config+binary fallback and shows the same integration instructions.Override behaviors and re-run
8.
--no-hooksflag — persistent opt-outI ran
but setup --no-hooks. It skips hook installation, persistsgitbutler.installHooks=false, and (post-rebase) itemizes which previously-installed managed hooks were cleaned up.9. Re-run detection — config opt-out preserved
After opting out (scenario 8), I ran
but setupagain. It shows the config-preserved opt-out without re-scanning and hints at--force-hooksto re-enable.10.
--force-hooks— override external manager detectionIn a repo where prek was previously detected (
installHooks=false), I ranbut 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-commitetc.11.
but teardownwith user backup — hooks restoredContinuing from scenario 10, I ran
but teardown. It reports each hook removal and showsRestored pre-commit (your original hook is back)for the hook that was backed up topre-commit-user.Edge case
12. Pre-existing plain user hook — partial install with actionable warning
I placed a plain
pre-commithook (not owned by any known manager) and ranbut setup. It installspost-checkoutandpre-push(each with✓ Installed …) and skipspre-commitwith a yellow warning including the--force-hooksoverride hint.Architecture
Notes
gix::discover_with_environment_overrides()in hook subcommandsByron flagged in his initial review that bare
gix::discover()is not the right call for hook subcommands — Git setsGIT_DIR/GIT_WORK_TREEbefore invoking hooks andgitoxidehasgix::open_with_environment_overrides()for exactly this. We usegix::discover_with_environment_overrides()which satisfies both requirements: it honours the git-provided environment variables when present (sogit commitinvocations work correctly), and falls back to a directory walk when invoked directly by an external manager like prek withoutGIT_DIRset.agents.mdsays not to re-discover repositories inside commands; these subcommands are a documented exception since they run as standalone hook subprocesses with noContext.Skill files updated
crates/but/skill/references/reference.mdwas updated with the newbut hooksubcommands and the--no-hooks/--force-hooksflags forbut setup, per theagents.mdconvention.Scope note
This PR is scoped to plumbing + CLI only, per the agreed plan in gitbutlerapp#12748:
but-hookscratebut doctor/but-diagnosebut hook install/uninstall/pre-commit/post-checkout/pre-push/statusbut setup/but teardown(delegate to universal)Note
Add external hook manager detection and ownership model to GitButler-managed hooks
but-hookscrate with managed hook installation, backup, and uninstall logic;setupandteardownnow report hook status including detection of external managers (e.g.,prek) and support--no-hooks/--force-hooksflagsDEFAULT_FORGE_FACTORYacross the desktop app with new project-scoped services (FORGE_INFO_SERVICE,PR_SERVICE,LISTING_SERVICE,REPO_SERVICE,CHECKS_MONITOR), injected at bootstrap and consumed throughout forge UI componentsstacks/stackDetailsendpoints toworkspaceDetailsbacked byhead_info, propagatingSegment-based data throughBranchView,BranchList,StackView, and related componentsbut-serverlocal-only hardening: removes tunnel/auth/remote features, restricts CORS to localhost, and exposes new forge and branch-integration IPC routesbut-transactioncrate providing a rebase-backed transactional context with oplog snapshot management for workspace mutationsDEFAULT_FORGE_FACTORYandForgePrServiceare removed from all call sites; downstream code that previously relied onDetailedPullRequest,stackDetails,push_stack_legacy, orunapply_stackmust migrate to new APIsMacroscope summarized ddc1156.