Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExports pnpm_config_pm_on_fail='download' when no explicit version is provided, upgrades bootstrap lockfiles to pnpm/@pnpm/exe 11.0.9, and expands CI tests to assert exact or minimum pnpm versions across OS and bin destinations. ChangesdevEngines onFail Configuration
Sequence Diagram(s)sequenceDiagram
participant runSelfInstaller as "runSelfInstaller"
participant process_env as "process.env"
participant bootstrap_pnpm as "bootstrap pnpm"
participant pnpm_runtime as "pnpm runtime"
runSelfInstaller->>process_env: set pnpm_config_pm_on_fail='download'
runSelfInstaller->>bootstrap_pnpm: invoke bootstrap selection/switch
bootstrap_pnpm->>pnpm_runtime: download/switch version
pnpm_runtime->>runSelfInstaller: report installed version
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/install-pnpm/run.ts`:
- Around line 70-74: The unconditional exportVariable('pnpm_config_pm_on_fail',
'download') is overriding project policy even when an explicit target version is
being installed; change it so the export is applied only during the bootstrap
case (no explicit target version). Locate the code that computes the
requested/self-update target (e.g., the variable or function that yields the
explicit version or "targetVersion"/"requestedVersion" used later for
self-update) and wrap the exportVariable('pnpm_config_pm_on_fail', 'download')
call in a conditional that only runs when that target is not present (or when in
bootstrap/install-with-action-version mode); ensure the export is not set once a
concrete version is determined before performing the self-update.
🪄 Autofix (Beta)
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: 0f6af364-d98d-4ef1-8d74-8c8d09e27f6a
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
.github/workflows/test.yamlsrc/install-pnpm/bootstrap/exe-lock.jsonsrc/install-pnpm/bootstrap/pnpm-lock.jsonsrc/install-pnpm/run.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Test with run_install (global, windows-latest)
- GitHub Check: Test bin_dest output points to requested version (10.33.2, ubuntu-latest)
- GitHub Check: Test devEngines.packageManager with onFail=error (>=9.15.0, ubuntu-latest)
- GitHub Check: Test devEngines.packageManager with onFail=error (>=9.15.0, macos-latest)
- GitHub Check: Test bin_dest output points to requested version (9.15.5, windows-latest)
- GitHub Check: Test with devEngines.packageManager (windows-latest, 9.15.5)
- GitHub Check: Test with standalone (windows-latest)
- GitHub Check: Test with default inputs (9.15.5, windows-latest)
- GitHub Check: Test packageManager field is respected (10.33.0, macos-latest)
- GitHub Check: Test with run_install (null, windows-latest)
- GitHub Check: Test with run_install (global, windows-latest)
- GitHub Check: Test devEngines.packageManager with onFail=error (>=9.15.0, ubuntu-latest)
- GitHub Check: Test devEngines.packageManager with onFail=error (>=9.15.0, windows-latest)
- GitHub Check: Test with devEngines.packageManager (macos-latest, >=9.15.0)
- GitHub Check: Test devEngines.packageManager with onFail=error (>=9.15.0, macos-latest)
- GitHub Check: Test packageManager field is respected (9.15.5, ubuntu-latest)
- GitHub Check: Test packageManager field is respected (10.33.0, windows-latest)
- GitHub Check: Test version input is actually installed (9.15.5, windows-latest)
- GitHub Check: Test with devEngines.packageManager (ubuntu-latest, >=9.15.0)
- GitHub Check: Test with devEngines.packageManager (windows-latest, >=9.15.0)
🔇 Additional comments (3)
src/install-pnpm/bootstrap/pnpm-lock.json (1)
8-14: Lockfile bump is consistent and looks correct.The root spec, resolved tarball, and integrity hash are aligned for
[email protected].src/install-pnpm/bootstrap/exe-lock.json (1)
8-145: Bootstrap exe lockfile refresh looks internally consistent.
@pnpm/exeand its platform artifacts are coherently pinned to11.0.9with matching resolved URLs and integrity fields..github/workflows/test.yaml (1)
301-350: Good regression coverage fordevEngines.packageManager.onFail=error.This job reproduces the failing path and validates both exact and ranged constraints across all three OS runners.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 94-143: Add a new matrix case that provides an explicit
packageManager version together with devEngines.packageManager.onFail="error"
and an intentionally mismatched REQUIRED so the action should fail; specifically
add a matrix entry (label like 'devEngines onFail=error, explicit mismatch')
whose manifest contains
'{"devEngines":{"packageManager":{"name":"pnpm","version":"9.15.5","onFail":"error"}}}'
but set version: '10.33.0'. Then update the test step "Test: pnpm reports the
pinned version" (the run block that reads REQUIRED from matrix.version and
compares actual="$(pnpm --version)") to handle this new case by asserting the
opposite: for the explicit onFail=error mismatch case ensure the test treats a
mismatch as the expected outcome (i.e., succeed when actual != REQUIRED and fail
if they match) so the workflow verifies that onFail="error" causes a failing
result when an explicit version is provided but doesn't match.
🪄 Autofix (Beta)
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: e00719cf-42ed-4ec5-89f8-4a52232d7e85
📒 Files selected for processing (1)
.github/workflows/test.yaml
📜 Review details
🔇 Additional comments (1)
.github/workflows/test.yaml (1)
51-73: Nice regression guard for PATH vsbin_destconsistency.The check around Line 51–Line 71 is strong: validating both
pnpmon PATH and"$BIN_DEST/pnpm"directly closes the Windows path-shadow/bin destination regressions cleanly.
Export pnpm_config_pm_on_fail=download so the bootstrap pnpm switches to the version pinned in devEngines.packageManager instead of throwing BAD_PM_VERSION on the user's first invocation.
Only export `pnpm_config_pm_on_fail=download` when the action is relying on bootstrap pnpm's runtime switch. When the user passes an explicit `version:` input, self-update aligns the binary and we should not silently override their `devEngines.packageManager.onFail = "error"` policy.
- Limit push trigger to master so PRs run jobs once instead of twice. - Fold test_default_inputs / test_dest / test_version_respects_request / test_bin_dest_output into one cross-OS `smoke` job (5 matrix entries: ubuntu × 2 versions + ubuntu custom-dest + macos + windows). - Fold test_package_manager_field / test_dev_engines / test_dev_engines_on_fail_error into one ubuntu-only `manifest_pin` job (6 matrix entries) — manifest handling is OS-independent. - Keep standalone and run_install but ubuntu-only. Total runs per PR push: ~88 → 14.
Adds a manifest_pin matrix entry combining `version: 10.33.0` with `devEngines.packageManager` pinned to 9.15.5/onFail=error. The action self-updates to 10.33.0, then `pnpm --version` must fail with BAD_PM_VERSION — confirming pnpm_config_pm_on_fail=download is scoped to the no-target-version branch and does not silently override the user's strict policy.
…vior The previous version of this test self-updated to pnpm 10.33.0 and expected pnpm --version to fail under devEngines.onFail=error. But pnpm v10 only reads the packageManager field, not devEngines, so the check never fired and the test failed. Switch the assertion to the contract the af8e203 scope fix actually enforces: when an explicit version: is supplied, pnpm_config_pm_on_fail must not be exported by the action. That's deterministic and pnpm-version-agnostic.
This was referenced May 11, 2026
3 tasks
zkochan
added a commit
to pnpm/pnpm
that referenced
this pull request
May 11, 2026
…11571) `pnpm` could hang for the lifetime of the worker pool after a command logically finished whenever the code path returned through `main` without `process.exit(...)`. `main.ts` only ran `finishWorkers()` inside the command-handler `finally` block. Short-circuit returns that came earlier — `--version`, `--help`, fatal config errors that surface before a command runs — bypassed it and left the integrity-resolution worker pool active. The CLI entry (`pnpm/src/pnpm.ts`) now runs `finishWorkers()` in its own `finally`, so every exit path tears down the pool. Cleanup rejections are swallowed so the `finally` never masks the real command result. ## Reproducer ```bash echo '{"devEngines":{"packageManager":{"name":"pnpm","version":"11.0.9","onFail":"download"}}}' > package.json time pnpm --version # before (Linux): prints 11.0.9, then hangs for the worker-pool lifetime # after: prints 11.0.9, exits in ~1-3 s ``` `switchCliVersion` resolves the integrity (spawning workers), finds nothing to swap (range/version already matches running pnpm), and returns through main's `--version` short-circuit. The unterminated workers used to keep the event loop alive. Surfaced in pnpm/action-setup#254 — bumping the action's bootstrap to 11.0.9 made every CI job using `devEngines.packageManager` with `onFail: "download"` hang.
This was referenced May 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pnpm_config_pm_on_fail=downloadfrom the action so the bootstrap pnpm switches versions viamanage-package-manager-versionsinstead of throwingBAD_PM_VERSIONwhen a project pinsdevEngines.packageManager.onFail = "error"without supplying aversion:input.test_dev_engines_on_fail_errorworkflow job (Linux/macOS/Windows, exact + range) that reproduces Action fails with devEngines.packageManager.onFail="error" when version input is omitted #252.Fixes #252.
Test plan
test_dev_engines_on_fail_errorpasses on all three OSestest_dev_engines(onFail=download) still passestest_package_manager_fieldstill passesSummary by CodeRabbit
Chores
Bug Fixes
Tests