Repository navigation
fix: don't write an install-time lefthook path into AI hook configs - #1544
Conversation
Confidence Score: 5/5The PR appears safe to merge with no outstanding correctness or repository-rule issues. No new changes were made after the previous review, and the sole previous finding was resolved after Greptile accepted anandghegde’s explanation and withdrew it. Reviews (3): Last reviewed commit: "fix: use plain lefthook in generated AI ..." | Re-trigger Greptile |
| // lives under node_modules, which is not on PATH, so it is called via npx. | ||
| func portableLefthookBin(exe string) string { | ||
| if slices.Contains(strings.Split(filepath.ToSlash(exe), "/"), "node_modules") { | ||
| return npxLefthookBin |
There was a problem hiding this comment.
Maybe it should be better just to rely on globally installed lefthook, rather than npx. I guess some users may want to use the lefthook binary from their repo, some will use pnpm for managing packages, and npx will download the lefthook for them.
What do you thin about having just lefthook here?
There was a problem hiding this comment.
Agreed, switched to plain lefthook in 0d70824. The npx guess was too narrow anyway (pnpm/yarn/bun users), and anyone running lefthook from a package manager can already set lefthook: pnpm exec lefthook etc., which is still used as is. Docs and the PR description are updated to match.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
0d70824 to
bf7d731
Compare
bf7d731 to
7a2229a
Compare
The generated AI hook settings (e.g. .claude/settings.json) are usually committed, but the command embedded the absolute path of the binary that ran install, which includes the home dir, OS/arch and version. Use `npx lefthook` when the running binary comes from node_modules and plain `lefthook` otherwise. The `lefthook` config option still wins.
7a2229a to
1e9d419
Compare
|
Thank you for bringing this change! |
Closes #1482
Context
lefthook installwith anai:section writes the result ofos.Executable()into the generated hook commands. With the npm package that ends up as something like/Users/me/project/node_modules/.pnpm/lefthook-darwin-arm64@2.1.10/.../bin/lefthook run validatein.claude/settings.json. That file is normally committed, so the command only works for whoever ran install, and breaks on other machines, on CI and after upgrades.Changes
lefthookconfig option is set it is still used as is.lefthook(per review). Projects that run lefthook from a package manager can setlefthook: npx lefthook/pnpm exec lefthook.lefthook run, so re-running install replaces them. Added a test for that.docs/configuration/ai.md.This is the AI-hook counterpart of #1398. #1526 (open) removes the
os.Executable()branch from the git hook shim; this PR touches different files and doesn't depend on it.Verified:
go test ./...passes,golangci-lint runreports no new issues.node_modules/lefthook-darwin-arm64/bin/wrote its absolute path; now it writeslefthook run validate, andlefthook: pnpm exec lefthookstill producespnpm exec lefthook run validate.