feat(cli): add vp hooks command for managing Git hooks (#2219) - #2341
feat(cli): add vp hooks command for managing Git hooks (#2219)#2341dennybiasiolli wants to merge 10 commits into
vp hooks command for managing Git hooks (#2219)#2341Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
9ac52bd to
db4b05e
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1d530ca12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function resolveHooksDir(dir?: string): string { | ||
| if (dir) { | ||
| return dir; | ||
| } | ||
| return getStoredHooksDir() ?? DEFAULT_HOOKS_DIR; |
There was a problem hiding this comment.
When setup runs at the repository root and a later vp hooks disable, enable, or status runs from a nested directory without --hooks-dir, this returns the stored .vite-hooks string relative to the new working directory. computeTarget() consequently looks for <nested-prefix>/.vite-hooks/_, treats the actual root .vite-hooks/_ as foreign, and disable can report success while leaving the root dispatcher and core.hooksPath active. Persist or reconstruct the setup-time Git prefix so remembered directories identify the same repository path regardless of the caller's current directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 613f68b. We now persist vp.hooks.prefix (git rev-parse --show-prefix at enable/disable time; . means worktree root) alongside vp.hooks.dir. Later commands without --hooks-dir resolve files and core.hooksPath from <toplevel>/<prefix>/<dir>, so disable from a nested cwd still tears down the root dispatcher. --hooks-dir stays cwd-relative, same as today’s install-from-subdir behavior. status/disable with nothing stored bind to the worktree root (and adopt an existing Vite+ core.hooksPath dispatcher) so a nested cwd does not poison the prefix.
| if (ownsHooksPath) { | ||
| const unsetError = unsetOwnedHooksPath(target); |
There was a problem hiding this comment.
When extensions.worktreeConfig is enabled, a repository-local Vite+ path can be hidden by a foreign worktree path; git config -h distinguishes --local as the repository config and --worktree as the per-worktree config. Because this condition only invokes unsetOwnedHooksPath() when the effective value is owned, disable leaves the hidden local .vite-hooks/_ value in place while deleting its dispatcher. If the worktree override is later removed, Git falls back to that now-broken Vite+ path. Inspect and unset owned values at each scope even when a foreign higher-precedence value is effective; unsetOwnedHooksPath() already preserves foreign scoped values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9d43c1a. disable always calls unsetOwnedHooksPath(), which already inspects local and worktree scopes and only unsets owned values. A foreign effective worktree path no longer skips cleanup of a hidden local Vite+ core.hooksPath.
| const target = rel ? `${rel}/${hooksDir}/_` : `${hooksDir}/_`; | ||
| const existingHooksPath = getEffectiveHooksPath(); | ||
| const userDisabled = isHooksUserDisabled(); | ||
| const dispatcherInstalled = existsSync(join(hooksDir, '_', 'h')); |
There was a problem hiding this comment.
If _ /h remains but one or more generated per-hook shims have been deleted during a partial cleanup, this still reports Dispatcher: installed. With core.hooksPath pointing at the target and a project-owned .vite-hooks/pre-commit present, vp hooks status therefore makes the setup appear active even though Git has no .vite-hooks/_/pre-commit entry point to invoke. Check the generated shim files, at least for each detected project hook, rather than using the helper file alone as proof that the dispatcher is installed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as-is. _/h is the dispatcher helper we own; enable always rewrites the full shim set, and disable removes the whole _/ tree. A missing named shim is a tampered/partial tree, not a state status needs to diagnose. vp config already uses _/pre-commit only as a first-run hint.
| // status 5 = key not found | ||
| if (result.status !== 0 && result.status !== 5) { |
There was a problem hiding this comment.
When the local Git config contains multiple vp.hooks.disabled values, git config --local --unset exits with status 5 because multiple values match, not only when the key is missing. Treating that status as success leaves the values intact, so vp hooks enable can install the dispatcher and report success while status still says disabled and later vp config skips installation. Use --unset-all for this preference or explicitly distinguish the multiple-value failure; git config -h documents --unset-all as removing all matches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 196a281. Preference and scoped core.hooksPath now use --unset-all. Git status 5 is still treated as success (missing key). Added a regression test with duplicate vp.hooks.disabled values.
| const args = mri(raw.slice(1), { | ||
| boolean: ['help'], | ||
| string: ['hooks-dir'], | ||
| alias: { h: 'help' }, | ||
| }); |
There was a problem hiding this comment.
The parser never validates args._, so an invocation such as vp hooks disable .custom-hooks silently ignores the directory operand and disables whichever stored or default directory resolveHooksDir() selects. This can remove the wrong dispatcher while leaving the intended custom hooks active, even though the command exits successfully. Reject unexpected positional operands and unknown options before calling setup, enable, or disable so malformed destructive commands cannot act on a fallback directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ca54fec. After help parsing, leftover args._ or unknown flags fail before enable/disable/status run. vp hooks disable .custom-hooks now errors and points at --hooks-dir.
| const existingHooksPath = getEffectiveHooksPath(); | ||
| const ownsHooksPath = | ||
| !!existingHooksPath && normalizeHooksPath(existingHooksPath) === normalizeHooksPath(target); | ||
| const foreignHooksPath = | ||
| !!existingHooksPath && normalizeHooksPath(existingHooksPath) !== normalizeHooksPath(target); |
There was a problem hiding this comment.
When core.hooksPath is an absolute path to this same dispatcher, such as /repo/.vite-hooks/_, comparing its normalized string with the generated relative target .vite-hooks/_ classifies it as foreign. vp hooks disable then leaves core.hooksPath unchanged but removes .vite-hooks/_, so Git is left pointing at a nonexistent dispatcher and status incorrectly labels the path as non-Vite+. Resolve both spellings against the Git worktree root before deciding ownership.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 44080a9. Ownership compares paths after resolving against the worktree root (and realpath, so /var vs /private/var matches). An absolute spelling of the same dispatcher is treated as owned for install refresh, status, and disable.
| description: 'Disable hooks: unset core.hooksPath, remove <dir>/_, persist preference', | ||
| }, | ||
| { | ||
| label: 'enable', |
There was a problem hiding this comment.
If enable is the same as setup, then there is no need to keep setup command. This way, vp hooks enable and vp hooks disable can correspond one-to-one.
There was a problem hiding this comment.
I kept also setup because of the list in #2219 , but I'm super-happy to drop setup so we can slightly reduce the surface of changes, working on it, and double checking all the other reviews from codex
There was a problem hiding this comment.
Dropped setup in 66af7c2. Surface is now enable / disable / status; enable both installs the first time and clears a disable preference.
d1d530c to
f96ecef
Compare
|
Rebased on top of the |
Implement pure hook lifecycle helpers on top of install(): user preference persistence (local git config), safe per-scope core.hooksPath unset, and status reporting. Teach install() and vp config to honor a disable preference and resolve the last-used hooks directory. Cover lifecycle, foreign path, worktree, stored custom dir, and unsafe-tree cases in unit tests; update the config help snapshot for the remembered hooks-dir default.
Add the hooks bin entry and register it from bin.ts so local vp can run setup/disable/enable/status. Bundle the entry with tsdown, document it in the CLI package build notes, and add a PTY lifecycle fixture for setup/status/disable/enable plus prepare-style config skip after disable.
Delegate hooks from the Rust global CLI to the JS implementation, and list the command in global help, the interactive picker, and the local NAPI help surface. Update top-level help snapshots and add vp hooks --help coverage for local and global flavors.
Add setup/disable/enable/status to the commit-hooks guide with a quick start, and point create, migrate, troubleshooting, and the guide index at the new commands so users can discover and operate them easily.
Keep enable/disable/status as the public surface. enable installs or refreshes the dispatcher and clears a disable preference.
Resolve relative and absolute hooksPath spellings against the git worktree root (via realpath) before deciding ownership, so disable and install do not treat /repo/.vite-hooks/_ as a foreign path.
f96ecef to
c088f14
Compare
Persist the setup-time git prefix with the hooks directory so enable, disable, status, and vp config resolve the same dispatcher from a nested cwd instead of treating root .vite-hooks/_ as foreign.
disable now always walks local and worktree scopes instead of only the effective value, so a foreign worktree path cannot leave a stale local Vite+ hooksPath behind after the dispatcher is removed.
git config --unset exits 5 when a key has multiple values, which we treated as success. Use --unset-all for the disable preference and scoped core.hooksPath so enable cannot leave a stale disabled state.
Positional directories and unknown flags were ignored, so `vp hooks disable .custom-hooks` could tear down the default dispatcher. Fail fast and point at --hooks-dir instead.
c088f14 to
ca54fec
Compare
2 participants
Closes #2219
Adds a dedicated
vp hookscommand so users can manage the Vite+ Git hookdispatcher without following the manual steps from the commit-hooks guide:
vp hooks setup— install or refresh the dispatcher (core.hooksPath+<dir>/_)vp hooks disable— tear down the dispatcher and persist the decision in localgit config so
prepare/vp configdo not reinstall itvp hooks enable— re-enable after disable (same as setup)vp hooks status— show preference,core.hooksPath, dispatcher, and project hooksProject-owned hooks (e.g.
.vite-hooks/pre-commit),stagedconfig, andpackage.jsonlifecycle scripts are left alone. Custom directories work via--hooks-dirand are remembered in local git config for later commands andvp config.Why
#2219: once hooks are set up there was no first-class way to remove them, so people
had to hand-edit
core.hooksPathand delete.vite-hooks/_. Maintainers preferreda
vp hookssurface (setup/disable/enable/status) over avp config --uninstall-hooksflag, with the disable decision persisted.Disclaimer: assisted by Grok 4.5 (xAI) while implementing and reviewing this change.