fix: keep the installed runtime authoritative against context-aware shims - #25
Conversation
…hims
pnpm 12 links global runtime bins as context-aware shims: running `node`
from `$PNPM_HOME/bin` inside a project switches to the version that project
pins in `devEngines.runtime`, fetching it on demand. That defeats the
runtime this action was asked to install — a matrix job asking for
`node@22` runs the repository's pinned version instead — and even when the
two versions agree pnpm materializes a second copy outside `$PNPM_HOME`.
Whenever the action installs a runtime, export `PNPM_CONFIG_GLOBAL_SHIMS`
with that runtime disabled (`{"node":false}`). The setting merges key-wise
over pnpm's defaults, so the runtimes the action did not install keep
theirs. A value the workflow set itself always wins, under either of the
two names pnpm reads it from, so opting back into the switching behaviour
stays possible.
`pnpm install --no-runtime` already expresses this for the install step;
the shims reintroduced the shadowing for every step after it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe runtime setup action now controls pnpm context-aware global shims. It disables shimming for installed runtimes by default, preserves workflow-provided settings, documents the behavior, and adds Ubuntu workflow validation for both paths. ChangesRuntime shim control
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yaml:
- Around line 470-513: Add a counterpart to the context-shims-opt-out coverage
that sets lowercase pnpm_config_global_shims in the action environment, then
verify the uppercase PNPM_CONFIG_GLOBAL_SHIMS remains unexported and node
resolves to the package.json project runtime v20.19.0. Keep the existing
uppercase-variable test intact and mirror its assertions for the lowercase
workflow override contract.
In `@action.yml`:
- Around line 36-39: Update the workflow documentation in action.yml lines 36-39
and README.md line 115 to state that the action’s default
PNPM_CONFIG_GLOBAL_SHIMS export occurs only when neither supported shim variable
has been set by the workflow; make README step 3 explicitly conditional on both
supported variables being unset.
In `@src/install-runtime/index.ts`:
- Around line 72-79: Update keepInstalledRuntimeAuthoritative to detect
configured environment variables by checking whether process.env[envName] is
undefined, rather than relying on truthiness, so explicitly empty values are
preserved and no override is exported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb4f1a29-c4c4-4a87-bf3c-1f350d2e34c2
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
.github/workflows/test.yamlREADME.mdaction.ymlsrc/install-runtime/index.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-11T16:19:49.450Z
Learnt from: zkochan
Repo: pnpm/setup PR: 1
File: src/cache-restore/run.ts:35-35
Timestamp: 2026-05-11T16:19:49.450Z
Learning: When using `actions/exec` (`getExecOutput` / `exec`), it is valid for the `commandLine` option to include both the command and its arguments in a single string (e.g., `getExecOutput('pnpm store path --silent')`). The library tokenizes `commandLine` internally (via `argStringToArray()`), so this behaves like passing an equivalent command + args array (e.g., `getExecOutput('pnpm', ['store','path','--silent'])`). In code reviews, do not flag this as incorrect—this matches documented behavior and a production-tested pattern.
Applied to files:
src/install-runtime/index.ts
🪛 ast-grep (0.45.0)
src/install-runtime/index.ts
[warning] 1-1: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 zizmor (1.29.0)
.github/workflows/test.yaml
[warning] 435-435: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 478-478: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🔇 Additional comments (2)
src/install-runtime/index.ts (1)
1-15: LGTM!.github/workflows/test.yaml (1)
427-469: LGTM!
PR Summary by QodoFix: keep installed runtime authoritative over pnpm 12 context shims
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
pnpm reads the setting from PNPM_CONFIG_GLOBAL_SHIMS or pnpm_config_global_shims, whichever comes first, so honouring only one of them would let the action override a workflow that used the other. Drive the opt-out job from a matrix over both names. Also record why an empty value counts as unset: pnpm ignores an empty value, so stepping aside for one would leave the shim enabled while looking like the workflow had opted in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pnpm 12 links global runtime bins as context-aware shims, so `node` inside this repository resolved to the version pinned in `devEngines.runtime` rather than the one the workflow asked for. The `Verify Node version` step caught it as soon as CI moved to pnpm 12.0.0-rc.2: it reported v26.7.0 where the Node 22 matrix entry expects v22.13.0, and the whole 22/24/26 matrix would otherwise have collapsed onto a single version. pnpm/setup#25 makes the action export `PNPM_CONFIG_GLOBAL_SHIMS` with the runtime it installed disabled, restoring what `pnpm install --no-runtime` already expressed for the install step. Pin every usage to the commit that carries it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pnpm 12 links global runtime bins as context-aware shims, so `node` inside this repository resolved to the version pinned in `devEngines.runtime` rather than the one the workflow asked for. The `Verify Node version` step caught it as soon as CI moved to pnpm 12.0.0-rc.2: it reported v26.7.0 where the Node 22 matrix entry expects v22.13.0, and the whole 22/24/26 matrix would otherwise have collapsed onto a single version. pnpm/setup#25 makes the action export `PNPM_CONFIG_GLOBAL_SHIMS` with the runtime it installed disabled, restoring what `pnpm install --no-runtime` already expressed for the install step. Pin every usage to the commit that carries it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
pnpm 12 links global runtime bins as context-aware shims: running
nodefrom$PNPM_HOME/bininside a project switches to the version that project pins indevEngines.runtime, fetching it on demand. That defeats the runtime this action was asked to install — a matrix job asking fornode@22runs the repository's pinned version instead — and even when the two versions agree, pnpm materializes a second copy outside$PNPM_HOME.This is exactly the shadowing the action already guards against for the install step with
pnpm install --no-runtime; the shims reintroduced it for every step after the action.Whenever the action installs a runtime, it now exports
PNPM_CONFIG_GLOBAL_SHIMSwith that runtime disabled:The setting merges key-wise over pnpm's defaults, so runtimes the action did not install keep theirs. A value the workflow set itself always wins — checked under both names pnpm reads (
PNPM_CONFIG_GLOBAL_SHIMSandpnpm_config_global_shims) — so opting back into the switching behaviour stays possible:Motivation
pnpm's own TS CI hit this the moment it moved to pnpm 12.0.0-rc.2: the
Verify Node versionstep assertspn node -vmatches the matrix Node that this action provisioned, and it reportedv26.7.0(the repo'sdevEngines.runtimepin) instead of the requestedv22.13.0— https://github.com/pnpm/pnpm/actions/runs/31318748252/job/93258395237. Without this change the whole 22/24/26 matrix silently collapses onto one version, and that applies to every repository that pins a runtime indevEngines.runtimeand sets a different one in CI.Verification
Ran the built action end-to-end locally (
dist/index.jswith the runner'sINPUT_*env encoding) against a workspace pinningdevEngines.runtimenode20.19.0, withruntime: node@22:GITHUB_ENVreceivesPNPM_CONFIG_GLOBAL_SHIMS={"node":false}.node --versionin the project →v22.23.2(the installed runtime).node --versionin the same project →v20.19.0— the bug.pnpm_config_global_shimspre-set, the action logs that it is leaving the setting alone and writes nothing toGITHUB_ENV.Two workflow jobs cover both directions:
runtime-survives-context-shimsasserts the installed runtime wins and the variable is exported, andcontext-shims-opt-outasserts a workflow-set value is untouched and pnpm still switches to the project's pin — the canary that keeps the first job from going vacuous if pnpm's defaults ever change.Written by an agent (Claude Code, claude-opus-5).
Summary by CodeRabbit
New Features
Documentation
Tests