feat(setup,cache-keys): add pnpm support#75
Conversation
Extend the shared CI action to support pnpm alongside npm and yarn, as the foundation for the org-wide npm/yarn -> pnpm migration (supply-chain hardening). cache-keys: - include pnpm-lock.yaml in the packageJsonLock hash (backwards compatible) setup: - detect pnpm (checked before yarn/npm — a repo may briefly have both lockfiles mid-migration) - enable Corepack (makes the packageManager field authoritative) - reorder so package-manager detection precedes cache-path computation, then add a pnpm-store-dir step and branch cache paths on the detected manager - install via `pnpm install --frozen-lockfile` (case stmt: pnpm | yarn | npm) - run `pnpm run --if-present prepare` for pnpm in the prepare/postinstall step - bump cache key prefix npm-- -> pkg-- and v2 -> v3 (cache bust) - npm audit signatures stays npm-only (unchanged) Backwards compatible: existing npm and yarn repos are unaffected — detection order and their install/prepare commands are preserved.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbitNew Features
WalkthroughAdds pnpm support to two composite GitHub Actions: cache-keys now includes Changespnpm Support Across Actions
Sequence DiagramsequenceDiagram
participant Workflow as GitHub Workflow
participant Detector as PM Detector
participant Corepack as Corepack
participant Cache as Actions/Cache
participant Installer as Installer
participant Prepare as Prepare Script
Workflow->>Detector: Determine package manager (check pnpm-lock/yarn.lock/package-lock)
Detector-->>Workflow: package-manager (pnpm|yarn|npm)
Workflow->>Corepack: corepack enable (if pnpm)
Corepack-->>Workflow: ready
Workflow->>Cache: Restore cache (key: pkg--*--v3)
Cache-->>Workflow: restored node_modules or pnpm store
Workflow->>Installer: Install dependencies (pm-specific)
Installer-->>Workflow: install complete
Workflow->>Prepare: Run prepare script (pm-specific)
Prepare-->>Workflow: prepare complete
Workflow->>Cache: Save cache (key: pkg--*--v3)
Cache-->>Workflow: cached
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
setup/action.yaml (1)
198-224:⚠️ Potential issue | 🟠 MajorFix
needsusage in composite-actionifcondition
setup/action.yamlis aruns.using: compositeaction, and theif:condition for “Run prepare npm script” usesneeds.initial-setup.outputs.scripts—needsisn’t supported for composite run-step conditions, so this logic can’t reliably evaluate. Use the composite step output already computed byid: scriptsinstead (steps.scripts.outputs.scripts).Proposed fix
- name: Run prepare npm script if: > steps.cache.outputs.cache-hit == 'true' && ( - contains(needs.initial-setup.outputs.scripts, ',prepare,') - || contains(needs.initial-setup.outputs.scripts, ',postinstall,') + contains(steps.scripts.outputs.scripts, ',prepare,') + || contains(steps.scripts.outputs.scripts, ',postinstall,') ) working-directory: ${{ inputs.working-directory }} shell: bash run: | case "${{ steps.package-manager.outputs['package-manager'] }}" in pnpm) pnpm run --if-present prepare ;; yarn) # yarn: # - doesn’t support `prepare` script (we use postinstall) # - doesn’t have --if-present if yarn run 2>/dev/null | grep -Eq '(^|\s)postinstall($|\s)' then yarn run postinstall fi ;; *) npm run --if-present prepare ;; esac🤖 Prompt for 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. In `@setup/action.yaml` around lines 198 - 224, The if condition for the "Run prepare npm script" step wrongly references needs.initial-setup.outputs.scripts (not available in composite actions); update the condition to use the composite step output id: scripts (i.e. steps.scripts.outputs.scripts) instead, keeping the rest of the logic (cache hit check and contains(...) checks) intact so the step evaluates correctly within the composite action.
🤖 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.
Outside diff comments:
In `@setup/action.yaml`:
- Around line 198-224: The if condition for the "Run prepare npm script" step
wrongly references needs.initial-setup.outputs.scripts (not available in
composite actions); update the condition to use the composite step output id:
scripts (i.e. steps.scripts.outputs.scripts) instead, keeping the rest of the
logic (cache hit check and contains(...) checks) intact so the step evaluates
correctly within the composite action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72d23743-d5d5-44ff-abac-ed0a364b586c
📒 Files selected for processing (2)
cache-keys/action.yamlsetup/action.yaml
…eview) Two fixes from the pre-merge review: - BLOCKER: the pnpm cache stores only the pnpm store (not node_modules), so a warm-cache run skipped `Install` and node_modules was never materialised. Run `pnpm install --frozen-lockfile` even on a cache hit for pnpm (fast/offline off the warm store); npm/yarn still skip install on a hit (they cache node_modules). - Scope `corepack enable` to pnpm repos only (was unconditional) — leaves npm/yarn repos, incl. any on Node < 16.9 without Corepack, completely unchanged.
Revert the pnpm branch added to "Run prepare npm script". That step's pre-existing `if:` references `needs.initial-setup.outputs.scripts`, which isn't available in a composite action, so the step never runs — and fixing that is out of scope for this backwards-compatible pnpm PR (it would change npm/yarn cache-hit behavior). pnpm doesn't need it anyway: `pnpm install` (which always runs for pnpm, incl. on a cache hit) executes prepare/postinstall itself. The step is now byte-identical to upstream. Follow-up: fix the condition to `steps.scripts.outputs.scripts` for npm/yarn separately.
|
@coderabbitai — thanks, that's a valid catch on the prepare step's Disposition for this PR: deferring the condition fix, reverting my change to that step.
So in |
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to open a follow-up GitHub issue to track the (´• ω •`) 🐇 ✏️ Learnings added
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
setup/action.yaml (1)
118-123: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider standardizing bracket notation for the hyphenated output name.
The file accesses
package-manageroutput inconsistently—dot notation here (lines 118, 123) vs bracket notation elsewhere (lines 170, 195). While both work, bracket notation (outputs['package-manager']) is preferred for hyphenated property names to avoid potential parsing ambiguity and improve consistency.♻️ Suggested standardization
- name: Enable Corepack # pnpm only — leaves npm/yarn repos (incl. any pinned to Node < 16.9 without # Corepack, or relying on system yarn resolution) completely unchanged. - if: steps.package-manager.outputs.package-manager == 'pnpm' + if: steps.package-manager.outputs['package-manager'] == 'pnpm' shell: bash run: corepack enable - name: Get pnpm store directory - if: steps.package-manager.outputs.package-manager == 'pnpm' + if: steps.package-manager.outputs['package-manager'] == 'pnpm' id: pnpm-store🤖 Prompt for 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. In `@setup/action.yaml` around lines 118 - 123, The workflow uses dot notation for the hyphenated output name (steps.package-manager.outputs.package-manager) inconsistently; update all accesses (including occurrences in the conditional "if: steps.package-manager.outputs.package-manager == 'pnpm'" and the subsequent Get pnpm store directory step) to use bracket notation outputs['package-manager'] (i.e., steps.package-manager.outputs['package-manager']) so hyphenated keys are accessed consistently and safely across the file.
🤖 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.
Outside diff comments:
In `@setup/action.yaml`:
- Around line 118-123: The workflow uses dot notation for the hyphenated output
name (steps.package-manager.outputs.package-manager) inconsistently; update all
accesses (including occurrences in the conditional "if:
steps.package-manager.outputs.package-manager == 'pnpm'" and the subsequent Get
pnpm store directory step) to use bracket notation outputs['package-manager']
(i.e., steps.package-manager.outputs['package-manager']) so hyphenated keys are
accessed consistently and safely across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cfd1838b-1f24-49e8-97db-38b8e1229792
📒 Files selected for processing (1)
setup/action.yaml
… nit) Standardize the hyphenated output access to bracket notation in the two pnpm if: conditions and the cache-paths shell check, matching the existing Install/ Verify steps. Cosmetic; both forms work. (The output-declaration value on the outputs: block keeps its upstream dot form.)
|
@coderabbitai review The bracket-notation nit was applied in |
|
(´• ω •`) 🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@setup/action.yaml`:
- Line 141: The shell word-splits the unquoted package.json path in the jq
invocation, breaking cache-path discovery when inputs.working-directory contains
spaces; update the jq command that uses '.workspaces // [] | .[] +
"/node_modules/"' so the package.json argument is quoted (e.g. wrap ${{
inputs.working-directory }}/package.json in quotes) to prevent splitting and
ensure jq receives the full path.
- Around line 148-153: The step "Set up GitHub npm registry authentication"
currently appends the PAT to the runner home ~/.npmrc; instead write the token
to a job-local temporary file (e.g., use RUNNER_TEMP or a created temp file) and
point npm/yarn/pnpm at that file by exporting/setting the appropriate userconfig
env var (npm_config_userconfig / NPM_CONFIG_USERCONFIG) or passing --userconfig
to the client; update the step that uses
inputs.github-npm-registry-personal-access-token to create the temp file, write
the auth line there, and ensure subsequent steps consume that temp config rather
than ~/.npmrc so the PAT is not persisted to the home directory.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8145475d-82b3-4417-8281-9693a8b51a75
📒 Files selected for processing (2)
cache-keys/action.yamlsetup/action.yaml
Quote ${{ inputs.working-directory }}/package.json in the cache-paths jq call so
a working-directory containing spaces doesn't word-split. Introduced by this PR's
reorder (the prefix was added to read the correct package.json). The other
CodeRabbit finding (PAT written to ~/.npmrc) is pre-existing/unchanged here and
deferred — changing the auth mechanism is out of scope for this pnpm PR.
|
@coderabbitai — both addressed: 1. Unquoted 2. PAT written to Net: the only change introduced by this PR that you flagged is fixed; the remaining item is pre-existing and deferred. |
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to open a single tracking issue covering both deferred items:
I can open it now and link it back to this PR. (´• ω •`) 🐇 ✏️ Learnings added
|
…deRabbit) Avoid persisting the GitHub Packages PAT in $HOME on self-hosted/persistent runners (composite actions have no post-job cleanup). Write auth to a job-local $RUNNER_TEMP/setup-registry.npmrc (chmod 600, seeded from any existing ~/.npmrc) and export NPM_CONFIG_USERCONFIG so npm + pnpm use it for the rest of the job. yarn (berry) is unaffected — it authenticates via GH_REGISTRY_TOKEN in the install step, not via npmrc. Also pass the PAT through env: instead of interpolating it into the run script.
…dex) - Truncate/seed the job-local npmrc on each invocation so re-running the action in one job can't append a duplicate _authToken line. - Reword the comment to the verified behavior (npm + pnpm 11.5.2 read the _authToken from the NPM_CONFIG_USERCONFIG file) instead of the npm-conf claim. Kept NPM_CONFIG_USERCONFIG (empirically confirmed to redirect pnpm 11.5.2 auth, incl. the _authToken); did NOT add npmrcAuthFile — pnpm 11.5.2 returns it undefined and ignores a token placed only there.
|
@coderabbitai review Current head is
One item I've deferred (happy to discuss): the prepare-step Please re-review the current head. |
|
Running a full re-review of head (´• ω •`) 🐇 ✅ Action performedFull review finished. |
marek-saji
left a comment
marek-saji
left a comment
There was a problem hiding this comment.
I’m not a fan of LLM verbosity. Especially when it comes to comments.
| # pnpm is checked first because during migration a repo may temporarily have | ||
| # both pnpm-lock.yaml and package-lock.json. |
There was a problem hiding this comment.
I don’t think we need this explanation here.
| - name: Enable Corepack | ||
| # pnpm only — leaves npm/yarn repos (incl. any pinned to Node < 16.9 without | ||
| # Corepack, or relying on system yarn resolution) completely unchanged. | ||
| if: steps.package-manager.outputs['package-manager'] == 'pnpm' | ||
| shell: bash | ||
| run: corepack enable |
There was a problem hiding this comment.
| - name: Enable Corepack | |
| # pnpm only — leaves npm/yarn repos (incl. any pinned to Node < 16.9 without | |
| # Corepack, or relying on system yarn resolution) completely unchanged. | |
| if: steps.package-manager.outputs['package-manager'] == 'pnpm' | |
| shell: bash | |
| run: corepack enable | |
| - name: Enable Corepack for pnpm | |
| if: steps.package-manager.outputs['package-manager'] == 'pnpm' | |
| shell: bash | |
| run: corepack enable |
| if [ "${{ steps.package-manager.outputs['package-manager'] }}" = "pnpm" ] | ||
| then | ||
| echo '${{ steps.pnpm-store.outputs.path }}' | ||
| else | ||
| { | ||
| echo 'node_modules/' | ||
| jq -r '.workspaces // [] | .[] + "/node_modules/"' "${{ inputs.working-directory }}/package.json" | ||
| echo '.yarn/install-state.gz' | ||
| } | sed 's~^~${{ inputs.working-directory }}/~' | ||
| fi |
There was a problem hiding this comment.
- if [ "${{ steps.package-manager.outputs['package-manager'] }}" = "pnpm" ]
+ if [ -n "${{ steps.pnpm-store.outputs.path }}" ]or even:
| if [ "${{ steps.package-manager.outputs['package-manager'] }}" = "pnpm" ] | |
| then | |
| echo '${{ steps.pnpm-store.outputs.path }}' | |
| else | |
| { | |
| echo 'node_modules/' | |
| jq -r '.workspaces // [] | .[] + "/node_modules/"' "${{ inputs.working-directory }}/package.json" | |
| echo '.yarn/install-state.gz' | |
| } | sed 's~^~${{ inputs.working-directory }}/~' | |
| fi | |
| { | |
| # npn and yarn | |
| echo 'node_modules/' | |
| jq -r '.workspaces // [] | .[] + "/node_modules/"' "${{ inputs.working-directory }}/package.json" | |
| # yarn | |
| echo '.yarn/install-state.gz' | |
| } | sed 's~^~${{ inputs.working-directory }}/~' | |
| # pnpm | |
| echo '${{ steps.pnpm-store.outputs.path }}' |
| if [ 0 -lt "$( jq -r '( .dependencies + .devDependencies ) // {} | to_entries | length' package.json )" ] | ||
| then | ||
| npm audit signatures | ||
| fi |
There was a problem hiding this comment.
Also supported by pnpm
| path: ${{ steps.cache-paths.outputs.paths }} | ||
| key: | | ||
| npm--${{ steps.cache-keys.outputs.os }}--${{ steps.cache-keys.outputs.node }}--${{ steps.cache-keys.outputs.packageJson }}--${{ steps.cache-keys.outputs.packageJsonLock }}--v2 | ||
| pkg--${{ steps.cache-keys.outputs.os }}--${{ steps.cache-keys.outputs.node }}--${{ steps.cache-keys.outputs.packageJson }}--${{ steps.cache-keys.outputs.packageJsonLock }}--v3 |
There was a problem hiding this comment.
I think npm prefix is still valid — all of them install from npm registry.
| # NOTE: pnpm is intentionally NOT handled here — `pnpm install` (which always runs for pnpm, | ||
| # incl. on a cache hit) executes the project's own prepare/postinstall. This step only matters | ||
| # for npm/yarn, where install is skipped on a cache hit. (Its pre-existing `needs.initial-setup` | ||
| # condition is a separate latent bug — see PR discussion; deferred to a focused follow-up.) |
There was a problem hiding this comment.
Let’s use if: to skip the step completely.
| # for npm/yarn, where install is skipped on a cache hit. (Its pre-existing `needs.initial-setup` | ||
| # condition is a separate latent bug — see PR discussion; deferred to a focused follow-up.) |
There was a problem hiding this comment.
What bug? What PR discussion?
| - name: Set up GitHub npm registry authentication | ||
| if: inputs.github-npm-registry-personal-access-token | ||
| shell: bash | ||
| env: | ||
| # Pass via env (not `${{ }}` interpolation into the script) so the token isn't rendered into | ||
| # the run command line. | ||
| GITHUB_NPM_REGISTRY_PERSONAL_ACCESS_TOKEN: ${{ inputs.github-npm-registry-personal-access-token }} | ||
| run: | | ||
| # Write registry auth to a job-local npmrc under $RUNNER_TEMP (wiped with the job) and point | ||
| # npm/pnpm at it via NPM_CONFIG_USERCONFIG — instead of persisting the PAT to ~/.npmrc, which | ||
| # would leave the token in $HOME on self-hosted/persistent runners (composite actions can't run | ||
| # a post-job cleanup step). Verified on pnpm 11.5.2 + npm: both read the registry _authToken from | ||
| # the NPM_CONFIG_USERCONFIG file. yarn (berry) reads the token from GH_REGISTRY_TOKEN in the | ||
| # install step, not from npmrc, so it is unaffected. | ||
| NPMRC="$RUNNER_TEMP/setup-registry.npmrc" | ||
| # Start from a clean file each invocation (seeded from any existing ~/.npmrc) so re-invoking the | ||
| # action in the same job doesn't append a duplicate token line. | ||
| if [ -f "$HOME/.npmrc" ]; then cp "$HOME/.npmrc" "$NPMRC"; else : > "$NPMRC"; fi | ||
| chmod 600 "$NPMRC" | ||
| printf '//npm.pkg.github.com/:_authToken=%s\n' "$GITHUB_NPM_REGISTRY_PERSONAL_ACCESS_TOKEN" >> "$NPMRC" | ||
| echo "NPM_CONFIG_USERCONFIG=$NPMRC" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
| - name: Set up GitHub npm registry authentication | |
| if: inputs.github-npm-registry-personal-access-token | |
| shell: bash | |
| env: | |
| # Pass via env (not `${{ }}` interpolation into the script) so the token isn't rendered into | |
| # the run command line. | |
| GITHUB_NPM_REGISTRY_PERSONAL_ACCESS_TOKEN: ${{ inputs.github-npm-registry-personal-access-token }} | |
| run: | | |
| # Write registry auth to a job-local npmrc under $RUNNER_TEMP (wiped with the job) and point | |
| # npm/pnpm at it via NPM_CONFIG_USERCONFIG — instead of persisting the PAT to ~/.npmrc, which | |
| # would leave the token in $HOME on self-hosted/persistent runners (composite actions can't run | |
| # a post-job cleanup step). Verified on pnpm 11.5.2 + npm: both read the registry _authToken from | |
| # the NPM_CONFIG_USERCONFIG file. yarn (berry) reads the token from GH_REGISTRY_TOKEN in the | |
| # install step, not from npmrc, so it is unaffected. | |
| NPMRC="$RUNNER_TEMP/setup-registry.npmrc" | |
| # Start from a clean file each invocation (seeded from any existing ~/.npmrc) so re-invoking the | |
| # action in the same job doesn't append a duplicate token line. | |
| if [ -f "$HOME/.npmrc" ]; then cp "$HOME/.npmrc" "$NPMRC"; else : > "$NPMRC"; fi | |
| chmod 600 "$NPMRC" | |
| printf '//npm.pkg.github.com/:_authToken=%s\n' "$GITHUB_NPM_REGISTRY_PERSONAL_ACCESS_TOKEN" >> "$NPMRC" | |
| echo "NPM_CONFIG_USERCONFIG=$NPMRC" >> "$GITHUB_ENV" | |
| - name: Set up GitHub npm registry authentication | |
| if: inputs.github-npm-registry-personal-access-token | |
| shell: bash | |
| env: | |
| GITHUB_NPM_REGISTRY_PERSONAL_ACCESS_TOKEN: ${{ inputs.github-npm-registry-personal-access-token }} | |
| run: | | |
| # Keep config where it’s wiped with the job to avoid it being persisted | |
| NPMRC="$RUNNER_TEMP/setup-registry.npmrc" | |
| touch "$NPMRC" | |
| chmod 600 "$NPMRC" | |
| { | |
| cat "$HOME/.npmrc" 2>/dev/null | |
| printf '//npm.pkg.github.com/:_authToken=%s\n' \ | |
| "$GITHUB_NPM_REGISTRY_PERSONAL_ACCESS_TOKEN" | |
| } > "$NPMRC" | |
| echo "NPM_CONFIG_USERCONFIG=$NPMRC" >> "$GITHUB_ENV" |
2 participants
Summary
Extend the shared CI action to support pnpm alongside npm and yarn — the Phase 1 foundation for the org-wide npm/yarn → pnpm migration (supply-chain hardening).
cache-keyspnpm-lock.yamlin thepackageJsonLockhash (backwards compatible —catignores missing files).setupcorepack enable(makes thepackageManagerfield authoritative).pnpm store pathstep and branch the cached paths on the detected manager.pnpm install --frozen-lockfile(case stmt:pnpm | yarn | npm).pnpm run --if-present preparefor pnpm in the prepare/postinstall step.npm--→pkg--andv2→v3(cache bust).npm audit signaturesstays npm-only (unchanged).Backwards compatible
Existing npm and yarn repos are unaffected — detection order and their install/prepare commands are preserved.
Versioning (post-merge)
Per the migration's §8.1 hardening, after merge we cut an immutable annotated tag (the repo's first semver tag, distinct from the moving
v1branch) and pin consumers to it (migration PRs bump@v1→@v1.2.0):Follow-ups (not in this PR)
setupinternally referencesverkstedt/actions/cache-keys@v1(still the moving branch). Pin it to@v1.2.0once that tag exists (or when theactionsrepo itself is migrated).actionlint(the repo'slint:actions) was not run locally (no Docker in the authoring env) — relying on this PR's CI for it.Test plan
🤖 Generated with Claude Code