Conversation
`pnpm self-update <version>` writes the target binary to
`${PNPM_HOME}/bin/`, leaving the bootstrap symlink at `${PNPM_HOME}/pnpm`
untouched. The `bin_dest` output was set to `${PNPM_HOME}`, so consumers
invoking `${{ steps.pnpm.outputs.bin_dest }}/pnpm` got the bootstrap
version (currently 11.0.4) instead of the version they requested.
PATH lookup hid the bug: `${PNPM_HOME}/bin` was prepended ahead of
`${PNPM_HOME}`, so `pnpm` resolved from PATH was the right one. Existing
version-respect tests only checked `pnpm --version`, not `bin_dest`.
Resolve `binDest` inside `runSelfInstaller` (target lives in
`${PNPM_HOME}/bin` after self-update, otherwise stays at `${PNPM_HOME}`)
and plumb it through to `setOutputs`. Add a regression test that invokes
`${bin_dest}/pnpm --version` directly across Linux/macOS/Windows.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR threads a structured installer result containing binDest through install, setOutputs, and runMain, and adds a CI job verifying the action's published bin_dest points to the requested pnpm version. ChangesBinary Destination Output Threading
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 unit tests (beta)
Comment |
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 199-200: The invocation that builds the actual pnpm version uses
an unquoted executable path (the expression that references
steps.pnpm.outputs.bin_dest), which will break if the bin_dest contains spaces;
update the assignment that sets actual to quote the executable path portion when
invoking pnpm (i.e., wrap the expanded ${ { steps.pnpm.outputs.bin_dest } }/pnpm
in quotes) so the shell treats the full path as a single token while still
passing --version.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c703c871-5db9-4c4c-a0ef-e0e4f23c3d8e
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (5)
.github/workflows/test.yamlsrc/index.tssrc/install-pnpm/index.tssrc/install-pnpm/run.tssrc/outputs/index.ts
Direct GitHub-expression interpolation of `${{ steps.pnpm.outputs.bin_dest }}`
into the bash script let bash eat the backslashes in the Windows path
(`C:Usersrunneradminsetup-pnpmnode_modules.binbin/pnpm`), failing with
"No such file or directory". Forward the value via env so the path
reaches bash unmangled.
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
${{ steps.pnpm.outputs.bin_dest }}/pnpmreturned the bootstrap pnpm (currently11.0.4) instead of the version requested via theversioninput.pnpm self-update <version>writes the target binary to${PNPM_HOME}/bin/, leaving the bootstrap symlink in${PNPM_HOME}/pnpmuntouched.bin_destwas set to${PNPM_HOME}, so direct invocations got the bootstrap.${PNPM_HOME}/binwas prepended ahead of${PNPM_HOME}, sopnpmresolved through PATH worked. Existing version-respect tests only checkedpnpm --version, never${bin_dest}/pnpm.binDestinsiderunSelfInstaller(target lives in${PNPM_HOME}/binafter self-update, otherwise stays at${PNPM_HOME}so the Corepack/packageManagerflow keeps working) and plumb it tosetOutputs.Test plan
test_bin_dest_outputworkflow job exercises${{ steps.pnpm.outputs.bin_dest }}/pnpm --versionfor9.15.5and10.33.2onubuntu-latest,macos-latest,windows-latest.${bin_dest}/pnpm --versionreported11.0.4forversion: 10.33.2. After fix: reports10.33.2.version-input branch (packageManager: pnpm@9.15.5):bin_deststill points to${PNPM_HOME}/,${bin_dest}/pnpm --versionreports9.15.5via Corepack.tsc --noEmitclean.dist/index.jsrebuilt.Summary by CodeRabbit
Bug Fixes
Outputs
Tests