Skip to content

fix: skip installing hooks when invoked inside node_modules - #153

Open
cpruijsen wants to merge 2 commits into
toplenboren:masterfrom
cpruijsen:fix/issue-82-acb011cd
Open

cpruijsen wants to merge 2 commits into
toplenboren:masterfrom
cpruijsen:fix/issue-82-acb011cd

Conversation

@cpruijsen

@cpruijsen cpruijsen commented Oct 12, 2026 •

Copy link
Copy Markdown

Fixes #82

cli.js now exits early with an [INFO] message when the working directory is inside node_modules, so a dependency that runs npx simple-git-hooks from its own postinstall can no longer overwrite or remove the parent project's git hooks.

Problem

In that case the CLI runs from <parent>/node_modules/<dep>. setHooksFromConfig reads the dependency's config, getGitProjectRoot walks up to the parent's .git, and the dependency's hooks are written there. Parent hooks missing from the dependency's config are deleted unless preserveUnused is set.

Change

  • simple-git-hooks.js: new exported isInsideNodeModules(directory=process.cwd()). It splits on / and \ and looks for an exact node_modules segment, as getProjectRootDirectoryFromNodeModules does.
  • cli.js: guard after the existing skipInstall() check, same shape.
  • postinstall.js is unchanged. It always runs inside node_modules and already resolves the project root through INIT_CWD.
  • Patch changeset.

The dependency's own developer still gets hooks in their checkout, because that checkout is not under node_modules.

I chose to skip rather than install into the parent root, because the config in the working directory belongs to the dependency, not the parent.

Known limit: a project that itself lives under a directory named node_modules can no longer run the CLI from there, because it looks the same as a dependency. I expect this to be rare. If you want an escape hatch, I can add an opt-out environment variable.

Tests

  • Unit tests for isInsideNodeModules: plain layout, nested pnpm store, Windows separators, a normal project, and a node_modules_backup segment that must not match.
  • An e2e test in simple-git-hooks.test.js builds a parent repo with an existing pre-push hook and a node_modules/some-dep package with its own config, then runs the CLI from the dependency directory.

Without the cli.js guard the e2e test fails: the CLI installs the dependency's pre-commit and deletes the parent's pre-push. With the guard it passes.

Summary by CodeRabbit

  • Bug Fixes
    • Hook installation is now skipped when the CLI runs from within a node_modules directory. The CLI displays an informational message, and existing hooks in the parent project remain unchanged.
    • Paths using either forward or backslashes are recognized, including nested dependency directories. Regular project paths and directory names that only contain node_modules are not treated as being inside it.

@changeset-bot

changeset-bot Bot commented Oct 12, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9c3119b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
simple-git-hooks Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3ad0f2ae-ff33-4390-8393-cf65bf13ba4c

📥 Commits

Reviewing files that changed from the base of the PR and between 39b29bb and 9c3119b.


📒 Files selected for processing (4)
  • .changeset/skip-inside-node-modules.md
  • cli.js
  • simple-git-hooks.js
  • simple-git-hooks.test.js

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.



📝 Walkthrough

Walkthrough

The CLI now checks whether its working directory is inside node_modules. If it is, the CLI logs a message and returns before installing hooks. The exported path check and tests cover path formats and the CLI behavior.

Changes

node_modules installation guard

Layer / File(s) Summary
Detect node_modules paths
simple-git-hooks.js, simple-git-hooks.test.js
Adds and exports isInsideNodeModules. Unit tests cover nested paths, Windows separators, ordinary paths, and names that only contain node_modules.
Skip CLI hook setup
cli.js, simple-git-hooks.test.js, .changeset/skip-inside-node-modules.md
The CLI logs a message and returns before hook setup when its working directory is inside node_modules. An integration test checks that the parent repository’s existing hook remains unchanged. A patch changeset describes the update.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: toplenboren


Merge Risk: ⚪ Minimal · up to 9c311

The change is mergeable after normal checks. The dependency-install scenario is covered by a test, though that test was not executed in this review.

Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: skipping hook installation when the CLI runs inside node_modules.
Linked Issues check Passed Issue #82 requires preventing a dependency from modifying the parent project's Git hooks. cli.js now calls isInsideNodeModules() after skipInstall() and exits before setHooksFromConfig() when …
Out of Scope Changes check Passed The changed helper, CLI guard, unit tests, end-to-end test, and patch changeset all support the issue #82 behavior. No unrelated change is demonstrated.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (1 skipped: 1 …

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 Biome (2.5.14)
cli.js

File contains syntax errors that prevent linting: Line 10: Illegal return statement outside of a function; Line 18: Illegal return statement outside of a function



Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When used in a dependency, the hooks installed into the parent package

1 participant