Optimize GitHub Actions caching and workflow triggers#1804
Open
pattonwebz wants to merge 6 commits into
Open
Conversation
jest-tests.yml: add node_modules cache layer so npm ci can skip re-linking on cache hit; switch from npm install to npm ci --prefer-offline. build-plugin-with-ref.yml: add vendor/ cache for Composer dependencies and pass --prefer-offline so composer install resolves from cache. phpunit.yml: remove monthly custom-cache-suffix that was busting the ramsey/composer-install cache on the 3rd of each month; add WordPress test library cache (/tmp/wordpress + /tmp/wordpress-tests-lib) keyed by WP version with run_id ensuring a fresh download on first run of each workflow but restoring from cache on retries and reruns. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
Contributor
📝 WalkthroughWalkthroughGitHub Actions workflows now tighten branch-based triggers and update CI dependency handling for Node, Composer, and WordPress test assets, with several install steps switching to cache-aware ChangesCI Workflow Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
npm install can rewrite package-lock.json when resolving dependencies, causing Copilot to commit lockfile changes on every PR. npm ci installs strictly from the existing lockfile without modifying it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/phpunit.yml (1)
78-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the cache-busting suffix on the highest-deps job The generated cache key still tracks the current lockfile, so without a suffix this workflow will keep reusing the same resolved vendor cache until
composer.jsonorcomposer.lockchanges.🤖 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 @.github/workflows/phpunit.yml around lines 78 - 88, The Install Composer dependencies step in the highest-deps job is missing the cache-busting suffix, so it can keep reusing the same resolved vendor cache. Update the ramsey/composer-install@v2 configuration in the phpunit workflow to keep the unique suffix for this job while preserving the selective composer-options behavior, so the cache key remains distinct from the normal dependency install path.
🤖 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/jest-tests.yml:
- Around line 29-40: The node_modules cache in the Jest workflow is ineffective
because the subsequent npm ci step deletes the restored directory before
reinstalling. Update the workflow around the Cache node_modules and Install
dependencies (skipping postinstall) steps so the cache is either removed
entirely or the install step is skipped when there is an exact cache hit; use
the existing actions/cache@v4 setup and npm ci block to implement the fix.
In @.github/workflows/phpunit.yml:
- Around line 89-98: The WordPress test library cache key currently uses
matrix.wp_version, which can be “latest” and cause stale cached installs to be
reused by install-wp-tests.sh. Update the cache configuration in the WordPress
test library cache step to key the cache by the resolved WordPress release
version instead of “latest,” or bypass caching when matrix.wp_version is latest,
so the cache follows the actual installed release.
---
Outside diff comments:
In @.github/workflows/phpunit.yml:
- Around line 78-88: The Install Composer dependencies step in the highest-deps
job is missing the cache-busting suffix, so it can keep reusing the same
resolved vendor cache. Update the ramsey/composer-install@v2 configuration in
the phpunit workflow to keep the unique suffix for this job while preserving the
selective composer-options behavior, so the cache key remains distinct from the
normal dependency install path.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d39939b-c3bb-4ad5-90fb-f7c62df4b95d
📒 Files selected for processing (3)
.github/workflows/build-plugin-with-ref.yml.github/workflows/jest-tests.yml.github/workflows/phpunit.yml
Reverts the removal of custom-cache-suffix from ramsey/composer-install per decision to keep monthly cache busting in place. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
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)
.github/workflows/copilot-setup-steps.yml (1)
66-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope
ignore-scriptsto this install only.
npm config set ignore-scripts truepersists for the rest of the job, so the repo’spostinstall/prepare step stays disabled until the config is reset. Usenpm ci --ignore-scripts ...instead.Suggested fix
- name: Install npm dependencies run: | # Skip postinstall scripts to avoid Cypress download issues in CI - npm config set ignore-scripts true - npm ci --prefer-offline --no-audit + npm ci --ignore-scripts --prefer-offline --no-audit🤖 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 @.github/workflows/copilot-setup-steps.yml around lines 66 - 70, The npm install step is changing a persistent config with `npm config set ignore-scripts true`, which leaves later lifecycle scripts disabled for the rest of the job. Update the `Install npm dependencies` step in the workflow to scope script skipping only to `npm ci` by using the per-command flag instead of setting global npm config, and keep the existing Cypress-related intent intact.
🤖 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 @.github/workflows/copilot-setup-steps.yml:
- Around line 66-70: The npm install step is changing a persistent config with
`npm config set ignore-scripts true`, which leaves later lifecycle scripts
disabled for the rest of the job. Update the `Install npm dependencies` step in
the workflow to scope script skipping only to `npm ci` by using the per-command
flag instead of setting global npm config, and keep the existing Cypress-related
intent intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd398c64-1f0a-4496-8722-89d85d6aa22c
📒 Files selected for processing (1)
.github/workflows/copilot-setup-steps.yml
All workflows that cache node_modules now use the same key pattern:
`${{ runner.os }}-node_modules-${{ hashFiles('**/package-lock.json') }}`
Previously each workflow used a different prefix (jest-node_modules-,
node-Linux-, Linux-node-) so they wrote to separate cache buckets and
could never share. Since lint-js runs on every develop push, it now
warms the shared bucket on the base branch. Any PR workflow that looks
up this key falls back to develop's cache and hits on the first run,
even before the PR branch has ever run a workflow.
Also switches make-pot.yml and lint-js.yml from npm install to npm ci
so they don't mutate package-lock.json, and changes make-pot.yml cache
path from ~/.npm to node_modules (the actual install directory) to match
the other workflows.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Workflows with both push and pull_request triggers were firing twice when a commit was pushed to a feature branch with an open PR: once for the push event and once for pull_request:synchronize. The concurrency group doesn't cancel between them because push uses refs/heads/branch and pull_request uses refs/pull/NNN/merge — different refs. Fix: scope the push trigger to develop/main/master/trunk only. Feature branches are covered by pull_request; direct pushes to integration branches (post-merge) are covered by push. No run is duplicated. Applies to: phpunit.yml, cs.yml, lint-php.yml, lint-js.yml Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pattonwebz
changed the title
node_modules cache + npm ci: npm ci deletes node_modules before reinstalling, so restoring the cache and then running npm ci was a no-op — the restored directory was thrown away immediately. Fix by adding id: cache-node-modules to each cache step and guarding the install with if: cache-hit != 'true'. On an exact key match the install is skipped entirely; on a partial or missing match npm ci runs normally and saves the result. Affects: jest-tests.yml, lint-js.yml, make-pot.yml, build-plugin-with-ref.yml (replaced the manual [ -d node_modules ] shell check with the proper cache-hit output). npm config set ignore-scripts true: this flag persists for the remainder of the job, disabling lifecycle scripts in later steps (e.g. Cypress setup). Replaced with the per-command --ignore-scripts flag on npm ci in jest-tests.yml and copilot-setup-steps.yml. WP test library cached under 'latest' key: install-wp-tests.sh returns immediately when /tmp/wordpress already exists, so a cached 'latest' run can keep serving an older WordPress release after upstream ships a new one. Added if: matrix.wp_version != 'latest' to skip caching for the three matrix jobs that use the moving target; the pinned 6.2 job continues to benefit from the cache. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
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 @.github/workflows/build-plugin-with-ref.yml:
- Around line 83-90: The Cache node_modules step in the build-plugin-with-ref
workflow is sharing a cache key with the ignore-scripts install path, so a cache
hit can skip the full npm install and leave postinstall work unapplied. Update
the cache namespace used by the cache-node-modules step so the full-install job
does not reuse the npm ci --ignore-scripts cache, or make this job always run
its install step before npm run dist. Keep the fix scoped to the workflow job
that builds the plugin with ref.
In @.github/workflows/lint-js.yml:
- Around line 31-42: Scope the shared node_modules cache by Node version in the
lint-js workflow so Node 20 and Node 22 do not reuse the same dependency tree.
Update the cache key in the Cache Node.js modules step to include the Node
major/version alongside the existing package-lock hash, and keep the restore key
compatible with that versioned prefix. Use the existing actions/cache@v4 step
and the cache-node-modules id as the place to make the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1cca63c-4869-4129-a988-e05b94bd59cf
📒 Files selected for processing (8)
.github/workflows/build-plugin-with-ref.yml.github/workflows/copilot-setup-steps.yml.github/workflows/cs.yml.github/workflows/jest-tests.yml.github/workflows/lint-js.yml.github/workflows/lint-php.yml.github/workflows/make-pot.yml.github/workflows/phpunit.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/copilot-setup-steps.yml
- .github/workflows/jest-tests.yml
- .github/workflows/phpunit.yml
Comment on lines
83
to
+90
| - name: Cache node_modules | ||
| id: cache-node-modules | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: node_modules | ||
| key: node-${{ runner.os }}-${{ hashFiles('**/package-lock.json') }} | ||
| key: ${{ runner.os }}-node_modules-${{ hashFiles('**/package-lock.json') }} | ||
| restore-keys: | | ||
| node-${{ runner.os }}- | ||
| ${{ runner.os }}-node_modules- |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
.github/workflows/build-plugin-with-ref.yml \
.github/workflows/make-pot.yml \
.github/workflows/lint-js.yml \
.github/workflows/jest-tests.yml
do
[ -f "$f" ] || continue
echo "== $f =="
rg -n -C2 'node_modules-|npm ci|ignore-scripts' "$f"
echo
doneRepository: equalizedigital/accessibility-checker
Length of output: 2305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json =="
cat -n package.json | sed -n '1,220p'
echo
echo "== build-plugin-with-ref workflow around dist =="
sed -n '100,140p' .github/workflows/build-plugin-with-ref.ymlRepository: equalizedigital/accessibility-checker
Length of output: 5572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scripts/prepare.sh =="
cat -n scripts/prepare.sh
echo
echo "== references to patch-package / prepare.sh / generated outputs =="
rg -n -C2 'patch-package|prepare\.sh|postinstall|generated' .github package.json scripts -g '!**/node_modules/**'Repository: equalizedigital/accessibility-checker
Length of output: 4104
Split the node_modules cache by install mode.
.github/workflows/build-plugin-with-ref.yml reuses the same cache key as workflows that populate node_modules with npm ci --ignore-scripts. Because a cache hit skips npm ci, this job can run npm run dist against a tree that never ran postinstall (patch-package && ./scripts/prepare.sh), which changes the built artifact. Give the full-install path its own cache namespace, or always run the install step here.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 85-85: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/build-plugin-with-ref.yml around lines 83 - 90, The Cache
node_modules step in the build-plugin-with-ref workflow is sharing a cache key
with the ignore-scripts install path, so a cache hit can skip the full npm
install and leave postinstall work unapplied. Update the cache namespace used by
the cache-node-modules step so the full-install job does not reuse the npm ci
--ignore-scripts cache, or make this job always run its install step before npm
run dist. Keep the fix scoped to the workflow job that builds the plugin with
ref.
Comment on lines
31
to
+42
| - name: Cache Node.js modules | ||
| id: cache-node-modules | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: node_modules | ||
| key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} | ||
| key: ${{ runner.os }}-node_modules-${{ hashFiles('**/package-lock.json') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-node- | ||
| ${{ runner.os }}-node_modules- | ||
| # The lint stage doesn't run the unit tests or use code style, so no need for PHPUnit, WPCS or phpcompatibility. | ||
| - name: 'Install NPM packages' | ||
| run: npm install --ignore-scripts | ||
| if: steps.cache-node-modules.outputs.cache-hit != 'true' | ||
| run: npm ci --prefer-offline --no-audit --ignore-scripts |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
.github/workflows/lint-js.yml \
.github/workflows/jest-tests.yml \
.github/workflows/build-plugin-with-ref.yml \
.github/workflows/make-pot.yml \
.github/workflows/copilot-setup-steps.yml
do
echo "===== $f ====="
rg -n -C2 'setup-node|node-version|actions/cache|node_modules-|npm ci' "$f" || true
echo
doneRepository: equalizedigital/accessibility-checker
Length of output: 4257
Scope the shared node_modules cache by Node version. The same key is reused by workflows pinned to Node 20 and Node 22, so a cache hit can skip npm ci with an incompatible dependency tree. Add the Node major/version to the key.
🤖 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 @.github/workflows/lint-js.yml around lines 31 - 42, Scope the shared
node_modules cache by Node version in the lint-js workflow so Node 20 and Node
22 do not reuse the same dependency tree. Update the cache key in the Cache
Node.js modules step to include the Node major/version alongside the existing
package-lock hash, and keep the restore key compatible with that versioned
prefix. Use the existing actions/cache@v4 step and the cache-node-modules id as
the place to make the change.
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
Five distinct improvements across eight workflow files:
1. Eliminate double-runs on PR pushes (
phpunit.yml,cs.yml,lint-php.yml,lint-js.yml)pushtriggers previously matched all branches ('*'orfeature/**). When a commit was pushed to a feature branch with an open PR, bothpushandpull_request:synchronizefired simultaneously. The concurrency group did not deduplicate them because each event uses a different ref (refs/heads/branchvsrefs/pull/NNN/merge). Both ran in full.Fix: scope
pushto integration branches only (develop,main,master,trunk). Feature branch pushes are now covered exclusively bypull_request.2. Cross-PR node_modules cache sharing (
lint-js.yml,jest-tests.yml,build-plugin-with-ref.yml,make-pot.yml)Each workflow previously used a different cache key prefix (
jest-node_modules-,node-Linux-,Linux-node-), so they wrote to separate buckets and could never share.lint-js.ymlruns on every push todevelop, which means it warms a cache on the base branch — but only workflows using the same key can benefit from it.Fix: unified all
node_modulescache keys to${{ runner.os }}-node_modules-${{ hashFiles('**/package-lock.json') }}. Now any PR's first workflow run finds the cachelint-js.ymlseeded ondevelop, regardless of which workflow looks it up first.Also changed the cache path in
make-pot.ymlfrom~/.npm(download cache) tonode_modules(the installed directory) to match the other workflows.3. WordPress test library cache (
phpunit.yml)Added
actions/cachefor/tmp/wordpress-tests-liband/tmp/wordpress, keyed on${{ runner.os }}-wordpress-${{ matrix.wp_version }}-. In practice, measured run times are nearly identical before and after (~6.7–7.7 runner-min), suggesting the WordPress test install was already fast on these runners. The cache is low-cost to maintain and may reduce variance on slower runners.4. Composer vendor cache for build workflow (
build-plugin-with-ref.yml)Added
actions/cachefor thevendor/directory keyed on${{ runner.os }}-vendor-${{ hashFiles('**/composer.lock') }}. This workflow only runs on labeled PRs and releases, so it previously always started cold.5.
npm install→npm ci(lint-js.yml,jest-tests.yml,make-pot.yml,copilot-setup-steps.yml)npm installresolves the dependency graph and can rewritepackage-lock.jsonif versions drift.npm ciinstalls strictly from the lockfile without touching it, preventing accidental lockfile commits (root cause of the Copilot lockfile-mutation problem).Estimated time savings per PR
All figures are runner-minutes (total compute summed across parallel jobs — the unit GitHub bills against your quota). Ranges are derived from actual min/max times measured across 8–10 recent workflow runs. Runner times vary due to network latency, runner CPU contention, and cache restore speed.
Measured runner-minutes per full run (min–max across recent history):
phpunit.ymllint-php.ymlcs.ymllint-js.ymljest-tests.ymlBefore each push to a feature branch with an open PR, all four CI workflows fired twice (
push+pull_request:synchronize). After, onlypull_requestfires once, andlint-jsis skipped entirely on non-JS pushes.Conservative saving per push = minimum "before" − maximum "after":
(6.6 + 0.55 + 1.22 + 0.43) × 2 − (7.7 + 0.72 + 1.48) = 17.6 − 9.9 = ~8 runner-minMaximum saving per push = maximum "before" − minimum "after":
(7.7 + 0.72 + 1.48 + 0.57) × 2 − (6.6 + 0.55 + 1.22) = 20.9 − 8.4 = ~13 runner-minThe double-run elimination is the dominant saving. The cross-PR node_modules cache provides an additional ~0.3–0.5 min per run on JS PRs (not included in the table above since it only applies when JS files change).
Test plan
phpunit,cs,lint-php,lint-jseach fire once, not twicepackage-lock.jsonhash; confirmjest-testsandlint-jsshow a cache hit on the first run (cache seeded bydevelop)build-plugin-with-ref.ymlon a labeled PR; confirmvendor/cache hit on repeat runspackage-lock.jsonchanges appear in any workflow workspace🤖 Generated with Claude Code
Summary by CodeRabbit