fix: move prepare lifecycle hook to explicit setup-hooks script - #334
Conversation
npm's prepare hook runs on every `npm install` in any environment, causing the package to silently install git hooks into any consumer's repository without their explicit consent. This is unexpected behavior for a plugin package consumed by end users. Rename prepare to setup-hooks so contributors can opt in explicitly by running `npm run setup-hooks` after cloning, as now documented in CONTRIBUTING.md. Co-Authored-By: Claude Code <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the git hook installation process from an automatic 'prepare' script to a manual 'setup-hooks' command in package.json. It also updates CONTRIBUTING.md with a new 'First-Time Setup' section to guide users through this manual process. Feedback was provided to replace an em-dash with a single dash in the documentation to adhere to stylistic preferences.
There was a problem hiding this comment.
Pull request overview
This PR makes git hook installation opt-in by removing use of npm’s prepare lifecycle hook, preventing hooks from being installed as a side-effect when end users install the package as a dependency.
Changes:
- Rename
package.jsonscript fromprepareto an explicitsetup-hooksscript. - Document a first-time contributor setup flow in
CONTRIBUTING.mdthat instructs runningnpm run setup-hooksmanually.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| package.json | Removes prepare lifecycle hook usage; introduces explicit setup-hooks npm script. |
| CONTRIBUTING.md | Adds “First-Time Setup” instructions to manually install git hooks via setup-hooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for this. The fix makes sense and the changes look clean. Two things from the bot reviews that should be addressed before merging:
Other than that, LGTM. |
- Remove pre-commit hook installation - it was just a no-op placeholder
('lib/ sync now handled by agent-core'), so installing it was pure
redundancy. Only the pre-push hook (preflight + /enhance reminder +
release tag validation) actually does anything.
- CONTRIBUTING.md: replace em-dash with single dash per workspace rule;
update text to reflect that only pre-push is installed now.
- docs/ARCHITECTURE.md: update setup-hooks.js comment - was 'npm prepare'
before xiaolai's PR; now correctly noted as manual.
|
Thanks @xiaolai! Took your patch and pushed two follow-ups in 71a5055:
Updated CONTRIBUTING.md prose to reflect that only pre-push is installed. Merging. |
Bug
package.jsoncurrently has:npm's
preparelifecycle hook runs automatically on everynpm install— including when end users install this package as a dependency in their own projects. This means git hooks (pre-commit, pre-push) get silently installed into any consuming project's.git/hooks/directory without explicit consent.This is a Medium-severity security concern: a package that installs git hooks as a side-effect of
npm installis unexpected behavior, and the installed hooks run arbitrary code before every commit and push in the consumer's repository.Fix
Rename
prepare→setup-hooksso hook installation is opt-in only:Contributors who want the git hooks run
npm run setup-hooksexplicitly after cloning. This is documented in a new "First-Time Setup" section inCONTRIBUTING.md.The hook content itself is benign (validators and /enhance prompts), but the automatic installation via the lifecycle hook is the issue — it should not happen without the developer's intent.
Changes
package.json: renameprepare→setup-hooksCONTRIBUTING.md: add "First-Time Setup" section with explicit opt-in instructions