Don't force past core.hooksPath guard in npm postinstall - #1475
Conversation
The npm postinstall/install scripts hardcoded `lefthook install -f`. The `--force` flag makes `ensureHooksPathUnset` skip the core.hooksPath guard added in evilmartians#1292 and install hooks into a stale custom hooks path anyway. This defeats the guard on every `npm install`. The concrete impact is teams migrating from Husky: Husky leaves `core.hooksPath=.husky/_` in each developer's local git config, so postinstall's forced install keeps writing hooks into `.husky/_` instead of `.git/hooks`, silently regenerating stale Husky hook files. The guard designed to catch exactly this never fires. Drop `-f` so the guard runs during postinstall. When a non-default core.hooksPath is set, lefthook now prints its guidance to run `lefthook install --reset-hooks-path` or `--force`, instead of silently forcing past it. The documented `--force` escape hatch is unchanged for users who intentionally want a custom hooks path.
|
|
||
| // run install | ||
| spawnSync(getExePath(), ['install', '-f'], { | ||
| spawnSync(getExePath(), ['install'], { |
There was a problem hiding this comment.
Existing-hook backup collision
When a non-Lefthook hook already has a corresponding .old backup, removing -f makes cleanHook reject the collision and abort hook creation; because the postinstall launchers discard the child status, npm still reports success while Lefthook hooks remain missing or partially installed.
There was a problem hiding this comment.
Good edge case to consider, but the same caveat in the PR description applies here as well: the runtime auto-sync path already rejects such .old backups and aborts hook creation. So this would bring the postinstall behavior in line with the runtime sync path.
It is a fairly narrow state, requiring both a foreign hook and a pre-existing .old backup. And in that state, letting the default behavior be silently deleting the user's backup does seem unfortunate.
On "npm still reports success while Lefthook hooks remain missing or partially installed", yes npm does discard the exit status, but the error (and guidance for fixing it) are still printed to the console.
There was a problem hiding this comment.
Those are all valid points. The runtime sync path already exhibits this same behavior — so the PR isn't introducing a new failure mode, it's making postinstall consistent with an existing one. And "fail visibly with printed guidance" is strictly better than "silently delete the user's .old backup." The narrowness of the state seals it.
I withdraw the concern. The edge case is real but the current behavior (with -f) is the worse outcome for it.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
Context
The npm
postinstallscripts hardcodelefthook install -f. Passing--forcemakesensureHooksPathUnsetskip thecore.hooksPathguard added in #1292 and install hooks into a non-default hooks path anyway. Because postinstall runs on everynpm install, the guard is silently defeated there, the one place it can't be interactively acknowledged.Concrete impact: teams migrating from Husky. Husky leaves
core.hooksPath=.husky/_in each developer's local git config, and that setting persists after the repo switches to lefthook. Everynpm installthen force-writes lefthook hooks into.husky/_instead of.git/hooks, silently regenerating stale Husky hook files. The guard designed to catch exactly this never fires.Worth noting: runtime auto-sync already behaves the "safe" way. On
lefthook run,syncHookscallsinstallHookswithforce=false, so a customcore.hooksPathalready gets no automatic re-sync today (the guard applies and sync is skipped). Dropping-ffrom postinstall simply makes the install path consistent with that existing auto-sync behavior—it does not newly break a supported workflow.Changes
Drop
-ffrom the three npm packaging install scripts:packaging/registries/npm/lefthook/postinstall.jspackaging/registries/npm-bundled/postinstall.jspackaging/registries/npm-installer/install.jsWhen a non-default
core.hooksPathis set, postinstall now surfaces lefthook's existing guidance (lefthook install --reset-hooks-pathor--force) instead of silently forcing past it. The documentedlefthook install --forceescape hatch is unchanged for users who intentionally want a custom hooks path.Possible follow-up (not required by this PR)
Users who intentionally use a custom
core.hooksPathwill need to runlefthook install --forcemanually after config changes. Since auto-sync already doesn't serve custom paths, this PR introduces no regression there, but if maintainers want to better support that workflow, a persistent opt-in (e.g. alefthook.ymlsetting or an env var pre-approving the custom path) could let postinstall install with--forceeach time instead.Confidence Score: 3/5
The PR should not merge until postinstall can enable the
core.hooksPathguard without silently failing installations that encounter an existing.oldhook backup.Removing the broad force flag activates more than the targeted hooks-path check; a reachable backup collision now aborts hook creation while all three npm launchers ignore the failed child status.
Files Needing Attention: packaging/registries/npm-bundled/postinstall.js, packaging/registries/npm-installer/install.js, packaging/registries/npm/lefthook/postinstall.js
Reviews (1): Last reviewed commit: "fix: don't force past core.hooksPath gua..." | Re-trigger Greptile