Skip to content

fix: append (not prepend) action node dir to PATH for npm bootstrap#241

Merged
zkochan merged 1 commit intomasterfrom
fix/240
May 2, 2026
Merged

fix: append (not prepend) action node dir to PATH for npm bootstrap#241
zkochan merged 1 commit intomasterfrom
fix/240

Conversation

@zkochan
Copy link
Copy Markdown
Member

@zkochan zkochan commented May 2, 2026

Summary

  • fix: use npm co-located with the action node binary #239 prepended dirname(process.execPath) to the child PATH so npm's #!/usr/bin/env node shebang could find node on GHE self-hosted runners where node isn't on PATH.
  • On runners with a prior setup-node-equivalent step, that prepend shadows the user-installed toolchain with the runner-bundled node (e.g. externals/node24/bin/), pairing the user's npm with a mismatched node — or picking up a broken npm that ships next to the runner-bundled node.
  • Append instead, so a user-installed toolchain on PATH keeps precedence. The runner's node dir remains as a fallback for the original v6 fails if npm does not exist on the runner (GHE) #234 case where node isn't on PATH at all.

Fixes #240

Test plan

  • GHE self-hosted runner with prior setup-node (Node v22 + matched npm) → bootstrap uses the user's node/npm, no module-resolution errors
  • GHE self-hosted runner without npm on PATH (original v6 fails if npm does not exist on the runner (GHE) #234 scenario) → bootstrap still works via the appended node dir
  • Standard GitHub-hosted Linux, macOS, Windows runners → no regression

Summary by CodeRabbit

  • Bug Fixes
    • Modified how Node is added to the system PATH to respect existing Node toolchain precedence.

#239 prepended `dirname(process.execPath)` to PATH so npm's
`#!/usr/bin/env node` shebang resolves on GHE self-hosted runners
where node isn't on PATH. But on runners with a prior `setup-node`
step, this shadowed the user's installed toolchain with the
runner-bundled node (e.g. externals/node24/bin), pairing the user's
npm with a mismatched node — or picking up a broken `npm` shipped
next to the runner-bundled node.

Append instead so a user-installed toolchain on PATH keeps
precedence; the runner's node dir remains as a fallback for the
original #234 case where node isn't on PATH at all.

Fixes #240
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5a6dbc19-6674-4680-8557-5be58eef222e

📥 Commits

Reviewing files that changed from the base of the PR and between 26f6d4f and c3e1112.

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**
📒 Files selected for processing (1)
  • src/install-pnpm/run.ts

📝 Walkthrough

Walkthrough

A single-file fix changes how pnpm-action adds its Node directory to PATH: appending instead of prepending it to preserve any pre-existing Node toolchain precedence (e.g., from prior setup steps), resolving conflicts on GHE runners with bundled Node versions.

Changes

PATH Precedence Fix

Layer / File(s) Summary
Environment Setup
src/install-pnpm/run.ts
NODE_DIR is now appended to PATH instead of prepended, allowing any user-installed Node toolchain on PATH to take precedence over the action's bundled Node directory while preserving #!/usr/bin/env node resolution.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A whisker-twitch to PATH's design,
Where Node versions now align,
Append instead of lead the way,
Let user's tools have rightful say,
The rabbit's fix makes runners play! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: appending (instead of prepending) the action's node directory to PATH for npm bootstrap, which directly addresses the core issue in the changeset.
Linked Issues check ✅ Passed The PR directly addresses issue #240 by implementing the exact fix proposed: appending instead of prepending the node directory to PATH, restoring user-installed toolchain precedence while preserving fallback functionality.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective of fixing PATH precedence for npm bootstrap in GHE self-hosted runners; no extraneous modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/240

Review rate limit: 9/10 reviews remaining, refill in 6 minutes.

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

@zkochan zkochan merged commit 8912a91 into master May 2, 2026
65 of 66 checks passed
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.

v6.0.4 issues with GHE runners

1 participant