fix: devendor tree-sitter-proto install lifecycle to prevent ENOTEMPTY on global upgrade#846
Conversation
… global upgrade PR #843's preinstall cleanup hook cannot address the reported bug because it runs on the NEW package's staging tree, not the OLD install being removed. Issue #836 still reproduces on 1.6.2-rc.8. Root cause: vendor/tree-sitter-proto was declared as `file:` dep with its own `dependencies` and `install` script, so npm created `vendor/tree-sitter-proto/node_modules/node-addon-api/` at install time, which blocked npm's rmdir on global upgrade. Changes: - Strip `dependencies` and `install` script from the vendored sub-package's package.json so npm no longer creates a nested node_modules or runs a lifecycle script under vendor/. - Hoist `node-addon-api` and `node-gyp-build` into gitnexus optionalDependencies; npm resolves them at the consumer's top level. - Add scripts/build-tree-sitter-proto.cjs modeled on patch-tree-sitter-swift.cjs. Runs at gitnexus postinstall, best-effort: skips cleanly on missing toolchain or --ignore-scripts so non-proto functionality keeps working. - Remove scripts/preinstall-cleanup.cjs — dead code; cannot run against the old install being removed. - Keep .npmignore entries from PR #843 (tarball hygiene, still correct). - Add explicit .gitignore rules for gitnexus/vendor/**/build and gitnexus/vendor/**/node_modules (closes the repo-side hygiene gap). - Add .github/workflows/ci-global-upgrade.yml: matrix smoke test that installs the previously-published rc globally, upgrades to the packed current branch, and verifies no vendor install-time artifacts survive. Runs on macOS (reporter's platform), Linux, and Windows. Also includes an --ignore-scripts degraded-mode lane. Wired into ci.yml gate. Plan: docs/plans/2026-04-15-002-fix-tree-sitter-proto-vendor-deps-plan.md Phase 1 (this commit) addresses the reported `node_modules/node-addon-api` hazard. Phase 2 (follow-up) will migrate to prebuildify + prebuilt .node binaries in the tarball — the 2026 canonical shape for tree-sitter grammars, which eliminates the postinstall compile path entirely. Refs #836
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…exus Three issues caught by CI on PR #846: 1. Concurrency linter rejected the `CIGU-` prefix (allowlist is `${{ github.workflow }}` or substring `CI-`). The literal-prefix guidance in ci.yml is specifically about disambiguating when reusable workflows run in nested contexts, and ci-global-upgrade doesn't need its own concurrency block at all — the caller (ci.yml) already governs concurrency for nested invocations. 2. `npm install` in gitnexus/ runs `prepare: node scripts/build.js`, which depends on gitnexus-shared/dist being built first. Other CI jobs handle this via the setup-gitnexus composite action. Use it here too (with build: 'false' — we only need the dep graph, then npm pack runs prepack which builds gitnexus itself). 3. Removed `pull_request` and `workflow_dispatch` triggers. The workflow is now pure `workflow_call` — invoked once from ci.yml via `uses:`. This avoids the duplicate-run problem where both the top-level pull_request trigger AND the nested workflow_call would fire on every PR.
Two fixes for ci-global-upgrade failures on PR #846: 1. The guard after the upgrade step was rejecting vendor/tree-sitter-proto/build/ in the global install. That was too strict. The original #836 bug was about vendor/tree-sitter-proto/node_modules/ specifically, not build/. The build/ directory appears because node-gyp-build compiles through the symlink npm creates at node_modules/gitnexus/node_modules/tree-sitter-proto, and its contents are plain .node, .obj, .lib files that rmdir handles without trouble. We know this empirically because the test got past the upgrade step in the run where the old vendor/node_modules was present. The guard now only flags nested node_modules, which is what the fix actually removes. 2. The Windows --ignore-scripts lane failed with ENOENT when npm tried to open the tarball. The path was computed in a bash step using $(pwd), which on Windows returns /d/a/... form, but npm install ran in the default cmd shell and received a mangled Windows path. Adding shell: bash to the install steps keeps path handling consistent.
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 6257 tests passed 97 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
@claude review this solution |
|
Claude finished @magyargergo's task in 1m 57s —— View job Code Review
This is a well-reasoned fix. The root cause diagnosis is correct: npm's What's goodThe The The CI workflow is a meaningful addition. The matrix across all three platforms, the The Issues and observations1. Build failure output is swallowed ( When 2. Vendor package.json description says "compatible with tree-sitter 0.25" ( The description reads: "ABI 14 build from coder3101/tree-sitter-proto latest grammar.js, compatible with tree-sitter 0.25". Gitnexus currently runs tree-sitter 0.21.1. The PR description explicitly notes that upstream main is now ABI 15 and would not load with 0.21. "Compatible with tree-sitter 0.25" is therefore either wrong or at least confusing for future maintainers. It should say "compatible with tree-sitter 0.21.x (ABI 14)" to be accurate. 3. CI comment about The comment says 4. The lane installs with 5. Minor:
SummaryThe fix is correct, the approach is sound, and the CI addition is a genuine improvement that fills a real gap. The issues above are minor: swallowed build output (#1) and the inaccurate vendor description (#2) are the most likely to cause real confusion later. None are blockers. Overall this is a clean, well-documented solution. |
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, #877 #878 #890) - AST-aware embedding chunking (#889) - jQuery / axios HTTP consumer detection (#887) - SemanticModel wired as first-class resolution input, SM-20 (#885) - ImportSemantics split into per-strategy hooks (#886) - Python dotted-import fix (#899); worker warnings non-terminal (#900 / #261); global-install ENOTEMPTY fixes (#843 #846); embeddings staleness fix (#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, #877 #878 #890) - AST-aware embedding chunking (#889) - jQuery / axios HTTP consumer detection (#887) - SemanticModel wired as first-class resolution input, SM-20 (#885) - ImportSemantics split into per-strategy hooks (#886) - Python dotted-import fix (#899); worker warnings non-terminal (#900 / #261); global-install ENOTEMPTY fixes (#843 #846); embeddings staleness fix (#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
…Y on global upgrade (abhigyanpatwari#846) * fix: devendor tree-sitter-proto install lifecycle to fix ENOTEMPTY on global upgrade PR abhigyanpatwari#843's preinstall cleanup hook cannot address the reported bug because it runs on the NEW package's staging tree, not the OLD install being removed. Issue abhigyanpatwari#836 still reproduces on 1.6.2-rc.8. Root cause: vendor/tree-sitter-proto was declared as `file:` dep with its own `dependencies` and `install` script, so npm created `vendor/tree-sitter-proto/node_modules/node-addon-api/` at install time, which blocked npm's rmdir on global upgrade. Changes: - Strip `dependencies` and `install` script from the vendored sub-package's package.json so npm no longer creates a nested node_modules or runs a lifecycle script under vendor/. - Hoist `node-addon-api` and `node-gyp-build` into gitnexus optionalDependencies; npm resolves them at the consumer's top level. - Add scripts/build-tree-sitter-proto.cjs modeled on patch-tree-sitter-swift.cjs. Runs at gitnexus postinstall, best-effort: skips cleanly on missing toolchain or --ignore-scripts so non-proto functionality keeps working. - Remove scripts/preinstall-cleanup.cjs — dead code; cannot run against the old install being removed. - Keep .npmignore entries from PR abhigyanpatwari#843 (tarball hygiene, still correct). - Add explicit .gitignore rules for gitnexus/vendor/**/build and gitnexus/vendor/**/node_modules (closes the repo-side hygiene gap). - Add .github/workflows/ci-global-upgrade.yml: matrix smoke test that installs the previously-published rc globally, upgrades to the packed current branch, and verifies no vendor install-time artifacts survive. Runs on macOS (reporter's platform), Linux, and Windows. Also includes an --ignore-scripts degraded-mode lane. Wired into ci.yml gate. Plan: docs/plans/2026-04-15-002-fix-tree-sitter-proto-vendor-deps-plan.md Phase 1 (this commit) addresses the reported `node_modules/node-addon-api` hazard. Phase 2 (follow-up) will migrate to prebuildify + prebuilt .node binaries in the tarball — the 2026 canonical shape for tree-sitter grammars, which eliminates the postinstall compile path entirely. Refs abhigyanpatwari#836 * fix(ci): ci-global-upgrade should be reusable-only and use setup-gitnexus Three issues caught by CI on PR abhigyanpatwari#846: 1. Concurrency linter rejected the `CIGU-` prefix (allowlist is `${{ github.workflow }}` or substring `CI-`). The literal-prefix guidance in ci.yml is specifically about disambiguating when reusable workflows run in nested contexts, and ci-global-upgrade doesn't need its own concurrency block at all — the caller (ci.yml) already governs concurrency for nested invocations. 2. `npm install` in gitnexus/ runs `prepare: node scripts/build.js`, which depends on gitnexus-shared/dist being built first. Other CI jobs handle this via the setup-gitnexus composite action. Use it here too (with build: 'false' — we only need the dep graph, then npm pack runs prepack which builds gitnexus itself). 3. Removed `pull_request` and `workflow_dispatch` triggers. The workflow is now pure `workflow_call` — invoked once from ci.yml via `uses:`. This avoids the duplicate-run problem where both the top-level pull_request trigger AND the nested workflow_call would fire on every PR. * fix(ci): relax vendor build/ guard and use bash shell on Windows Two fixes for ci-global-upgrade failures on PR abhigyanpatwari#846: 1. The guard after the upgrade step was rejecting vendor/tree-sitter-proto/build/ in the global install. That was too strict. The original abhigyanpatwari#836 bug was about vendor/tree-sitter-proto/node_modules/ specifically, not build/. The build/ directory appears because node-gyp-build compiles through the symlink npm creates at node_modules/gitnexus/node_modules/tree-sitter-proto, and its contents are plain .node, .obj, .lib files that rmdir handles without trouble. We know this empirically because the test got past the upgrade step in the run where the old vendor/node_modules was present. The guard now only flags nested node_modules, which is what the fix actually removes. 2. The Windows --ignore-scripts lane failed with ENOENT when npm tried to open the tarball. The path was computed in a bash step using $(pwd), which on Windows returns /d/a/... form, but npm install ran in the default cmd shell and received a mangled Windows path. Adding shell: bash to the install steps keeps path handling consistent.
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (abhigyanpatwari#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, abhigyanpatwari#877 abhigyanpatwari#878 abhigyanpatwari#890) - AST-aware embedding chunking (abhigyanpatwari#889) - jQuery / axios HTTP consumer detection (abhigyanpatwari#887) - SemanticModel wired as first-class resolution input, SM-20 (abhigyanpatwari#885) - ImportSemantics split into per-strategy hooks (abhigyanpatwari#886) - Python dotted-import fix (abhigyanpatwari#899); worker warnings non-terminal (abhigyanpatwari#900 / abhigyanpatwari#261); global-install ENOTEMPTY fixes (abhigyanpatwari#843 abhigyanpatwari#846); embeddings staleness fix (abhigyanpatwari#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
Closes #836. PR #843 tried to fix this earlier but it did not work. The reporter confirmed the same error still happens on 1.6.2-rc.8.
Why PR #843 did not work
PR #843 added a preinstall script that cleans up leftover files from a previous install. The problem is that preinstall runs on the new package being installed, not on the old one being removed. By the time npm is unpacking the new version, it has already failed trying to remove the old one. The preinstall script never gets a chance to help.
The other change in PR #843 was adding entries to .npmignore. That was a good idea for a different reason (keeping the published tarball clean), but it was not what caused this bug. I verified by downloading the actual 1.6.2-rc.8 tarball from npm and confirming those directories are not in it. The hazard is created at install time, not shipped with the package.
What actually causes the bug
Our package.json declares tree-sitter-proto as a local file dependency that lives under vendor/tree-sitter-proto/. That vendored folder has its own package.json declaring its own dependencies (node-addon-api and node-gyp-build) plus an install script that compiles a native binary.
When npm processes a file dependency, it treats it like any other package. It installs the transitive dependencies into a nested node_modules folder inside vendor/tree-sitter-proto/, then runs the install script there, which writes a build/ folder in the same place.
When a user runs npm install -g gitnexus@new over an existing global install, npm first tries to remove the old gitnexus directory. On macOS it cannot remove vendor/tree-sitter-proto/node_modules/node-addon-api cleanly and aborts with ENOTEMPTY.
Every other tree-sitter grammar we use (c, cpp, python, rust, swift, kotlin, dart, and so on) avoids this because they install into node_modules/, which npm manages and knows how to clean up. tree-sitter-proto was the only one writing into the vendor/ folder.
The fix
The goal is simple. Nothing should write anything under vendor/ during install. That is the same invariant every other grammar in the project already follows.
Step 1. In vendor/tree-sitter-proto/package.json, remove the dependencies block and remove the install script. Without those, npm has no reason to create a nested node_modules or run anything in the vendor folder.
Step 2. In gitnexus/package.json, move node-addon-api and node-gyp-build into optionalDependencies at the top level. They resolve from gitnexus/node_modules/ the normal way. The version ranges match what the vendored package declared before, so nothing changes about which versions get installed.
Step 3. Add a new postinstall script, scripts/build-tree-sitter-proto.cjs. It compiles the native binary by running node-gyp-build inside node_modules/tree-sitter-proto/ (which npm manages), never under vendor/. The script is modeled on the existing patch-tree-sitter-swift.cjs and follows the same best effort pattern: if the compiler toolchain is missing, or the user installed with --ignore-scripts, or the optional dependency was skipped, the script warns and exits cleanly. Non-proto functionality keeps working.
Step 4. Delete scripts/preinstall-cleanup.cjs. It was dead code and keeping it around made it look like the bug was fixed.
Step 5. In gitnexus/package.json, chain the new script after the Swift patch in postinstall, and remove the preinstall entry. The .npmignore entries from PR #843 stay because they are still good defense in depth.
Step 6. Add two rules to the root .gitignore for gitnexus/vendor/**/build and gitnexus/vendor/**/node_modules. The generic node_modules rule already covered the nested case, but build was not explicitly ignored. I checked git history and those paths have never been accidentally committed, so this is preventative.
Step 7. Add a new CI workflow, ci-global-upgrade.yml. It runs a matrix across macOS, Ubuntu, and Windows that installs the previously published 1.6.2-rc.8 globally, packs the current branch, installs the packed tarball over the old version, and verifies the CLI still works. It also checks that vendor/tree-sitter-proto/node_modules and vendor/tree-sitter-proto/build do not exist after install. A second lane installs with --ignore-scripts and confirms the CLI boots in degraded mode. This workflow is wired into ci.yml as a required gate.
How I verified locally on Windows
About the ABI
Worth noting because it almost sent me down the wrong path. gitnexus runs tree-sitter 0.21.1, which can only load grammars built with ABI 13 or 14. Every grammar in the project is ABI 14, including the vendored tree-sitter-proto. I briefly considered switching to upstream coder3101/tree-sitter-proto via git URL, but their current main is now ABI 15, which would not load. So vendoring the source and compiling at install time is the right shape for our current runtime. When we upgrade tree-sitter to 0.22 or later, that changes.
What comes later
The 2026 standard for tree-sitter grammars is to ship prebuilt native binaries in the npm tarball using prebuildify. Every first party tree-sitter grammar does this. Upstream tree-sitter-proto is set up for it but has never actually been published to npm. Migrating us to that shape would eliminate the install-time compile entirely, including the --ignore-scripts edge case. It needs a platform matrix in CI and a binary publication workflow, so it is a bigger change and I am saving it for a follow-up.
Test plan