fix: Selected version in package.json not being used by action#246
Open
djmurphy32 wants to merge 8 commits intopnpm:masterfrom
Open
fix: Selected version in package.json not being used by action#246djmurphy32 wants to merge 8 commits intopnpm:masterfrom
djmurphy32 wants to merge 8 commits intopnpm:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe ChangesVersion Candidate Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Review rate limit: 8/10 reviews remaining, refill in 11 minutes and 16 seconds. 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 the current code and only fix it if needed.
Inline comments:
In `@src/install-pnpm/run.ts`:
- Around line 128-141: The code is currently treating empty strings as
"unspecified" by using filter(v => !!v); instead, explicitly validate each
candidate (version, packageManagerVersion, devEnginesVersion) for blank/empty
values and throw a descriptive Error when any candidate exists but is an empty
string (e.g., "packageManagerVersion is blank" or "devEnginesVersion is blank");
then build the Set only from validated non-blank values, keep the existing
conflict check using definedVersions, and return the single value as
before—reference the variables definedVersions, version, packageManagerVersion,
and devEnginesVersion to locate and update the logic.
🪄 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: 38d66848-c55b-4f52-b4ec-deb25fc38c29
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/install-pnpm/run.ts
Comment on lines
+128
to
+141
| const definedVersions = new Set([version, packageManagerVersion, devEnginesVersion].filter(v => !!v)) | ||
| if (definedVersions.size > 1) { | ||
| throw new Error(`Multiple conflicting pnpm versions specified: | ||
| - version ${version ?? "undefined"} in the GitHub Action config with the key "version" | ||
| - version ${packageManagerVersion ?? "undefined"} in the package.json with the key "packageManager" | ||
| - version ${devEnginesVersion ?? "undefined"} in the package.json with the key "devEngines.packageManager" | ||
| Remove conflicting versions to avoid version mismatch errors like ERR_PNPM_BAD_PM_VERSION`) | ||
| } | ||
|
|
||
| // pnpm will automatically download and switch to the right version | ||
| if (typeof packageManager === 'string' && packageManager.startsWith('pnpm@')) { | ||
| if (definedVersions.size === 0) { | ||
| return undefined | ||
| } | ||
|
|
||
| if (devEngines?.packageManager?.name === 'pnpm' && devEngines.packageManager.version) { | ||
| return undefined | ||
| } | ||
|
|
||
| if (!GITHUB_WORKSPACE) { | ||
| throw new Error(`No workspace is found. | ||
| If you've intended to let pnpm/action-setup read preferred pnpm version from the "packageManager" field in the package.json file, | ||
| please run the actions/checkout before pnpm/action-setup. | ||
| Otherwise, please specify the pnpm version in the action configuration.`) | ||
| } | ||
|
|
||
| throw new Error(`No pnpm version is specified. | ||
| Please specify it by one of the following ways: | ||
| - in the GitHub Action config with the key "version" | ||
| - in the package.json with the key "packageManager" | ||
| - in the package.json with the key "devEngines.packageManager"`) | ||
| return definedVersions.values().next().value |
There was a problem hiding this comment.
Do not silently ignore empty pnpm version values.
filter(v => !!v) drops empty strings, so a malformed packageManager: "pnpm@" or empty devEngines.packageManager.version is treated the same as “no version specified” and the action falls back to the bootstrap pnpm instead of failing fast. Please validate candidates explicitly and throw on blank values.
Proposed fix
- const definedVersions = new Set([version, packageManagerVersion, devEnginesVersion].filter(v => !!v))
+ const candidateVersions = [version, packageManagerVersion, devEnginesVersion]
+ if (candidateVersions.some(v => v === '')) {
+ throw new Error('Invalid pnpm version specified')
+ }
+ const definedVersions = new Set(candidateVersions.filter((v): v is string => v !== undefined))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const definedVersions = new Set([version, packageManagerVersion, devEnginesVersion].filter(v => !!v)) | |
| if (definedVersions.size > 1) { | |
| throw new Error(`Multiple conflicting pnpm versions specified: | |
| - version ${version ?? "undefined"} in the GitHub Action config with the key "version" | |
| - version ${packageManagerVersion ?? "undefined"} in the package.json with the key "packageManager" | |
| - version ${devEnginesVersion ?? "undefined"} in the package.json with the key "devEngines.packageManager" | |
| Remove conflicting versions to avoid version mismatch errors like ERR_PNPM_BAD_PM_VERSION`) | |
| } | |
| // pnpm will automatically download and switch to the right version | |
| if (typeof packageManager === 'string' && packageManager.startsWith('pnpm@')) { | |
| if (definedVersions.size === 0) { | |
| return undefined | |
| } | |
| if (devEngines?.packageManager?.name === 'pnpm' && devEngines.packageManager.version) { | |
| return undefined | |
| } | |
| if (!GITHUB_WORKSPACE) { | |
| throw new Error(`No workspace is found. | |
| If you've intended to let pnpm/action-setup read preferred pnpm version from the "packageManager" field in the package.json file, | |
| please run the actions/checkout before pnpm/action-setup. | |
| Otherwise, please specify the pnpm version in the action configuration.`) | |
| } | |
| throw new Error(`No pnpm version is specified. | |
| Please specify it by one of the following ways: | |
| - in the GitHub Action config with the key "version" | |
| - in the package.json with the key "packageManager" | |
| - in the package.json with the key "devEngines.packageManager"`) | |
| return definedVersions.values().next().value | |
| const candidateVersions = [version, packageManagerVersion, devEnginesVersion] | |
| if (candidateVersions.some(v => v === '')) { | |
| throw new Error('Invalid pnpm version specified') | |
| } | |
| const definedVersions = new Set(candidateVersions.filter((v): v is string => v !== undefined)) | |
| if (definedVersions.size > 1) { | |
| throw new Error(`Multiple conflicting pnpm versions specified: | |
| - version ${version ?? "undefined"} in the GitHub Action config with the key "version" | |
| - version ${packageManagerVersion ?? "undefined"} in the package.json with the key "packageManager" | |
| - version ${devEnginesVersion ?? "undefined"} in the package.json with the key "devEngines.packageManager" | |
| Remove conflicting versions to avoid version mismatch errors like ERR_PNPM_BAD_PM_VERSION`) | |
| } | |
| if (definedVersions.size === 0) { | |
| return undefined | |
| } | |
| return definedVersions.values().next().value |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/install-pnpm/run.ts` around lines 128 - 141, The code is currently
treating empty strings as "unspecified" by using filter(v => !!v); instead,
explicitly validate each candidate (version, packageManagerVersion,
devEnginesVersion) for blank/empty values and throw a descriptive Error when any
candidate exists but is an empty string (e.g., "packageManagerVersion is blank"
or "devEnginesVersion is blank"); then build the Set only from validated
non-blank values, keep the existing conflict check using definedVersions, and
return the single value as before—reference the variables definedVersions,
version, packageManagerVersion, and devEnginesVersion to locate and update the
logic.
djmurphy32
force-pushed
the
fix-installing-new-versions
branch
from
f33147b to
da754f8
Compare
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.
This fixes an regression that was introduced in this change. In my testing pnpm was not using the version specified in the package.json from either the
packageVersionordevEnginesas expected, and was instead always using the version that was bootstrapped or the version specified in the action's options if defined.This changes the logic to update the installed pnpm version to match the specified version by any of the action's version option,
packageVersion, ordevEnginesand will error if there is conflicting versions between any of the 3.This should resolve #225 and resolve #231
Summary by CodeRabbit
Bug Fixes
Refactor