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:
📝 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.
- Drop the floating "pnpm is checked first" block and fold the one useful sentence into the detection script, where it applies. - Rename the step to "Enable Corepack for pnpm" and drop its explanation, per suggestion. - Key the cache-path branch off `steps.pnpm-store.outputs.path` being set rather than re-deriving the package manager. Kept the if/else rather than the unconditional variant so pnpm still caches only the store: node_modules under pnpm is a symlink farm and install always runs for it, so archiving it would add a lot of file operations for nothing. Say so if you'd rather have the flatter version anyway. - Take the suggested npmrc form verbatim. It also chmods before the token is written rather than after, and it is shorter. - Revert the cache-key prefix to `npm--`. See the note below. - `audit signatures` is supported by pnpm too, so the provenance step now runs for pnpm as well, dispatching on the detected manager. yarn (berry) has no equivalent and stays excluded. - Skip "Run prepare npm script" via `if:` instead of explaining the pnpm case in the body. - The vague comment is gone, and the bug it gestured at is fixed rather than described: `needs.initial-setup.outputs.scripts` does not resolve inside a composite action, so that condition was always false and the step never ran on a cache hit. It now uses this action's own `steps.scripts.outputs.scripts`, which is what `outputs.scripts` already exposes. Two things worth a second look: 1. Fixing the `needs.` reference means the prepare/postinstall step starts running for npm/yarn repos on a cache hit, which it has not been doing. That is the intended behaviour, but it is a behaviour change beyond pnpm support, so it should be a deliberate call rather than a drive-by. 2. With the prefix back to `npm--`, the restore-keys can still fall back to a pre-pnpm cache entry for a repo that has just migrated, because only the exact key carries `--v3`. Harmless in practice (pnpm always installs), but if you want it airtight the manager could go into the key itself. actionlint clean.
…RE MERGE. DO NOT MERGE THIS COMMIT. Revert it once validation is done. Why it is needed. This PR cannot be exercised from a consumer repo without it. Pointing a repo's ci.yaml at this branch changes which reusable workflow runs, but that workflow then loads `verkstedt/actions/setup@v1`, hardcoded, 12 times. So the consumer runs the branch's workflow against v1's action and fails with v1's error text. Confirmed in hebammen-tiim-app run 30654569524. The same applies one level deeper, and it hid a real gap: setup/action.yaml calls `cache-keys@v1`, and this PR changes cache-keys too (packageJsonLock now hashes pnpm-lock.yaml). An earlier validation run that invoked setup@this-branch directly was therefore still running the OLD cache-keys, so half of this PR had never executed at all. Repointed here, all to @chore/pnpm-support: .github/workflows/ci.yaml 12x setup + 1x cache-keys .github/workflows/chromatic.yaml 1x setup setup/action.yaml 1x cache-keys Left on @v1 deliberately: create-github-app-token and notify-status, which this PR does not touch. If this lands on v1, every verkstedt repo's CI starts depending on a feature branch that can be deleted. Hence: revert first, then merge. actionlint clean.
The Build StoryBook step ran `"$PACKAGE_MANAGER" run build-storybook --
--webpack-stats-json`. npm and yarn consume the `--` and forward the flag; pnpm
passes it through literally, so the script received it as a positional argument
and storybook exited 1:
$ storybook build -- --webpack-stats-json
error: too many arguments for 'build'. Expected 0 arguments but got 1.
Found by running the real Chromatic workflow against a pnpm repo, which is only
possible with the temporary internal pins on this branch. Setup itself passed;
this was the next step along.
Verified both forms locally on pnpm 11.5.2: with `--` it fails as above, without
it the build succeeds AND emits storybook-static/preview-stats.json, so the flag
is genuinely honoured rather than quietly dropped. That file is what Chromatic's
TurboSnap consumes, so dropping the flag silently would have been worse than the
crash.
This is the only `run ... -- --` pattern in the shared workflows.
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