fix: honor devEngines.packageManager.onFail=error (#252) by zkochan · Pull Request #254 · pnpm/action-setup · GitHub
Skip to content

fix: honor devEngines.packageManager.onFail=error (#252) - #254

Merged
zkochan merged 6 commits into
masterfrom
fix/252
May 10, 2026
Merged

zkochan merged 6 commits into
masterfrom
fix/252

Conversation

@zkochan

@zkochan zkochan commented May 10, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes #252.

Test plan

  • CI test_dev_engines_on_fail_error passes on all three OSes
  • Existing test_dev_engines (onFail=download) still passes
  • Existing test_package_manager_field still passes

Summary by CodeRabbit

  • Chores

    • Bundled pnpm updated to 11.0.9 across platform variants and lockfiles.
  • Bug Fixes

    • Installer now forces download/switch when no explicit pnpm version is provided so onFail="error"/"download" semantics won’t block initial setup.
  • Tests

    • CI adds matrixed checks for pinned vs. ranged packageManager settings (including onFail:error), compares PATH vs. installed pnpm, and requires exact pnpm versions (9.15.0/9.15.5) in key steps.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 10, 2026 •

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 91ab88e and 1d7402b.

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**
📒 Files selected for processing (4)
  • .github/workflows/test.yaml
  • src/install-pnpm/bootstrap/exe-lock.json
  • src/install-pnpm/bootstrap/pnpm-lock.json
  • src/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 pnpm@11.0.9.

src/install-pnpm/bootstrap/exe-lock.json (1)

8-145: Bootstrap exe lockfile refresh looks internally consistent.

@pnpm/exe and its platform artifacts are coherently pinned to 11.0.9 with matching resolved URLs and integrity fields.

.github/workflows/test.yaml (1)

301-350: Good regression coverage for devEngines.packageManager.onFail=error.

This job reproduces the failing path and validates both exact and ranged constraints across all three OS runners.

Comment thread src/install-pnpm/run.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between af8e203 and 134fc98.

📒 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 vs bin_dest consistency.

The check around Line 51–Line 71 is strong: validating both pnpm on PATH and "$BIN_DEST/pnpm" directly closes the Windows path-shadow/bin destination regressions cleanly.

Comment thread .github/workflows/test.yaml
zkochan added 6 commits May 11, 2026 01:49
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.
@zkochan
zkochan merged commit 1155470 into master May 10, 2026
17 checks passed
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.
dbmikus pushed a commit to gofixpoint/amika that referenced this pull request Oct 23, 2026
Bumps [pnpm/action-setup](https://github.com/pnpm/action-setup) from 5.0.0 to 6.1.0.

## Release notes

Sourced from pnpm/action-setup's releases.

### v6.1.0

### What's Changed
- feat: support pnpm v12 by `@zkochan` in pnpm/action-setup#288

Full Changelog: pnpm/action-setup@v6.0.10...v6.1.0

### v6.0.10

### What's Changed
- docs(README): point users to the successor pnpm/setup action by `@BlackHole1` in pnpm/action-setup#282
- refactor: introduce restore keys for cache by `@SukkaW` in pnpm/action-setup#280
- ci: use pnpm 11 for `pr-check` by `@jamietanna` in pnpm/action-setup#284
- fix: update pnpm to v11.19.0 by `@jamietanna` in pnpm/action-setup#283
- docs: update README to include devEngines.packageManager by `@nemchik` in pnpm/action-setup#273

### New Contributors
- `@jamietanna` made their first contribution in pnpm/action-setup#284
- `@nemchik` made their first contribution in pnpm/action-setup#273

Full Changelog: pnpm/action-setup@v6...v6.0.10

### v6.0.9

### What's Changed
- fix: update pnpm to v11.7.0 by `@zkochan` in pnpm/action-setup#267

Full Changelog: pnpm/action-setup@v6...v6.0.9

### v6.0.8

### What's Changed
- docs(README): fix `cache_dependency_path` type by `@haines` in pnpm/action-setup#257
- fix: drop patchPnpmEnv so standalone+self-update works on Windows by `@zkochan` in pnpm/action-setup#258
- fix: update pnpm to 11.1.1 by `@mungodewar` in pnpm/action-setup#248

### New Contributors
- `@mungodewar` made their first contribution in pnpm/action-setup#248

Full Changelog: pnpm/action-setup@v6.0.7...v6.0.8

### v6.0.7

### What's Changed
- fix: honor devEngines.packageManager.onFail=error (#252) by `@zkochan` in pnpm/action-setup#254
- fix: restore inputs from state in post by `@haines` in pnpm/action-setup#255
- fix: self-update bootstrap to packageManager-pinned version (#233) by `@zkochan` in pnpm/action-setup#256

### New Contributors
- `@haines` made their first contribution in pnpm/action-setup#255

Full Changelog: pnpm/action-setup@v6.0.6...v6.0.7

... (truncated)

## Commits
- `ea17c68` feat: support pnpm v12 (#288)
- `0977fd9` docs: Update README to include devEngines.packageManager (#273)
- `48261ac` fix: update pnpm to v11.19.0 (#283)
- `75677f7` ci: use pnpm 11 for `pr-check` (#284)
- `769ae71` refactor: introduce restore keys for cache (#280)
- `6fed91f` docs(README): point users to the successor pnpm/setup action (#282)
- `0ebf471` fix: update pnpm to v11.7.0 (#267)
- `0e279bb` fix: update pnpm to 11.1.1 (#248)
- `3e83581` fix: drop patchPnpmEnv so standalone+self-update works on Windows (#258)
- `551b42e` docs(README): fix `cache_dependency_path` type (#257)
- Additional commits viewable in compare view
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.

Action fails with devEngines.packageManager.onFail="error" when version input is omitted

1 participant