Skip to content

fix(ci): include package manifests in node_modules cache keys #2472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 2, 2026
Merged

fix(ci): include package manifests in node_modules cache keys #2472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ jobs:
# runner's own persistence, and npm ci always deletes+reinstalls node_modules by design -- so
# neither the runner nor npm ci gives node_modules any real cross-run reuse on its own. This
# explicit restore/save pair (via GitHub's own cache service, not the wiped local disk) fills that
# gap: an exact package-lock.json match skips npm ci entirely. Keyed separately per fork/trusted
# gap: an exact manifest+lockfile match skips npm ci entirely. Keyed separately per fork/trusted
# (see the runs-on expression above) because self-hosted's Docker image and GitHub's ubuntu-latest
# image are not guaranteed binary-compatible for native modules (sharp, workerd, fsevents, ...) --
# crossing them could load an incompatible native binary. Fork PRs get read-only cache tokens (a
Expand All @@ -152,10 +152,12 @@ jobs:
path: |
node_modules
apps/gittensory-ui/node_modules
# hashFiles('.nvmrc') matters as much as the lockfile: a Node bump with no lockfile change
# would otherwise still hit and silently reuse node_modules whose native addons (sharp,
# workerd, fsevents) were compiled against the OLD Node's ABI.
key: npm-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('package-lock.json') }}
# hashFiles('.nvmrc') matters as much as the package manifests and lockfile: a Node bump
# with no lockfile change would otherwise still hit and silently reuse node_modules whose
# native addons (sharp, workerd, fsevents) were compiled against the OLD Node's ABI. The
# manifests matter too because npm ci validates package.json/package-lock.json consistency
# and runs lifecycle scripts from package.json; a package.json-only change must not skip it.
key: npm-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json') }}
- name: Install dependencies (retry on transient failures)
if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }}
run: |
Expand Down Expand Up @@ -313,8 +315,9 @@ jobs:
uses: actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0
with:
path: review-enrichment/node_modules
# Same Node-version guard as the root cache key above -- REES runs on the same pinned .nvmrc.
key: npm-rees-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('review-enrichment/package-lock.json') }}
# Same Node-version and manifest guards as the root cache key above -- REES runs on the same
# pinned .nvmrc and has its own package.json lifecycle/install validation.
key: npm-rees-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('review-enrichment/package.json', 'review-enrichment/package-lock.json') }}
- name: REES install
if: ${{ (github.event_name == 'push' || needs.changes.outputs.rees == 'true') && steps.rees-node-modules-cache.outputs.cache-hit != 'true' }}
run: npm run rees:install
Expand Down
12 changes: 9 additions & 3 deletions test/unit/ci-dependency-cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function jobSteps(workflow: Record<string, unknown>, jobName: string): Array<Rec

// actions/checkout wipes node_modules on every run (git clean -ffdx) and npm ci always deletes+reinstalls
// it by design, so neither gives node_modules any real cross-run reuse -- these restore/save pairs (via
// GitHub's own cache service) fill that gap: an exact package-lock.json match skips the install step
// GitHub's own cache service) fill that gap: an exact manifest+lockfile match skips the install step
// entirely. Cache-save is placed right after a successful install (not as an automatic post-job hook), so
// a job that fails installing never reaches the save step -- a broken node_modules can never get cached.
describe("CI dependency-install caching", () => {
Expand All @@ -43,7 +43,11 @@ describe("CI dependency-install caching", () => {
const restoreWith = record(restore.with, "restore.with");
expect(String(restoreWith.path)).toContain("node_modules");
expect(String(restoreWith.path)).toContain("apps/gittensory-ui/node_modules");
expect(String(restoreWith.key)).toContain("hashFiles('package-lock.json')");
expect(String(restoreWith.key)).toContain("hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json')");
expect(String(restoreWith.key)).toContain("package.json");
expect(String(restoreWith.key)).toContain("apps/*/package.json");
expect(String(restoreWith.key)).toContain("packages/*/package.json");
expect(String(restoreWith.key)).toContain("package-lock.json");
// A Node bump (.nvmrc) with no lockfile change must still bust the cache -- otherwise a hit would
// silently reuse node_modules whose native addons were compiled against the OLD Node's ABI.
expect(String(restoreWith.key)).toContain("hashFiles('.nvmrc')");
Expand All @@ -70,7 +74,9 @@ describe("CI dependency-install caching", () => {
const restore = step(steps, "Restore review-enrichment node_modules cache");
const restoreWith = record(restore.with, "restore.with");
expect(restoreWith.path).toBe("review-enrichment/node_modules");
expect(String(restoreWith.key)).toContain("hashFiles('review-enrichment/package-lock.json')");
expect(String(restoreWith.key)).toContain("hashFiles('review-enrichment/package.json', 'review-enrichment/package-lock.json')");
expect(String(restoreWith.key)).toContain("review-enrichment/package.json");
expect(String(restoreWith.key)).toContain("review-enrichment/package-lock.json");
expect(String(restoreWith.key)).toContain("hashFiles('.nvmrc')");

const install = step(steps, "REES install");
Expand Down
Loading