fix(ci): cache node_modules across runs to skip npm ci on an exact hit - #2446
Conversation
actions/checkout's default clean:true runs `git clean -ffdx` before every checkout, wiping node_modules on every run even though the self-hosted "gittensory" runner containers are persistent VPS processes, not fresh machines. npm ci also unconditionally deletes and reinstalls node_modules by design regardless of what's on disk. Together these meant node_modules got zero cross-run reuse no matter how long the runner container stays alive. Add an actions/cache/restore + actions/cache/save pair (GitHub's own cache service, not local disk) around both the root npm ci and review-enrichment's separate npm ci (it isn't an npm workspace member, so it has its own package-lock.json and needs its own cache entry). The install step is skipped entirely when the restore is an exact hit; the save step is placed immediately after a successful install (not as an automatic post-job hook), so a job that fails installing never reaches it -- a broken/partial node_modules can never get written to the cache for a future run to inherit. Cache keys are scoped separately per fork-vs-trusted (mirroring the existing runs-on split): the self-hosted runner's Docker image and GitHub's ubuntu-latest are not guaranteed binary-compatible for native modules (sharp, workerd, fsevents, @sentry/cli all compile platform-specific binaries), so crossing them could load an incompatible binary. Fork PRs get read-only cache tokens (documented actions/cache behavior) and can never write a "fork"-keyed entry -- they keep doing a full npm ci exactly as before, no regression on that no-retry population. Adversarial review caught one real bug before this shipped: the first draft's cache key only hashed the lockfile, so a Node version bump (.nvmrc) with no lockfile change would still hit and silently reuse node_modules whose native addons were compiled against the OLD Node's ABI. Both keys now also hash .nvmrc. Added test/unit/ci-dependency-cache.test.ts to pin the restore/skip/save wiring for both install paths, including the .nvmrc fix and that REES's build/test step still runs unconditionally (independent of whether install ran or was skipped this run).
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 01:37:33 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 5 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2446 +/- ##
=======================================
Coverage 95.91% 95.91%
=======================================
Files 224 224
Lines 25240 25240
Branches 9177 9177
=======================================
Hits 24210 24210
Misses 417 417
Partials 613 613 🚀 New features to boost your workflow:
|
Summary
actions/checkout's defaultclean: truerunsgit clean -ffdxbefore every checkout, wipingnode_moduleson every run even though the self-hosted "gittensory" runner containers are persistent VPS processes, not fresh machines.npm cialso unconditionally deletes and reinstallsnode_modulesby design regardless of what's already on disk. Together these meantnode_modulesgot zero cross-run reuse no matter how long the runner container stays alive.actions/cache/restore+actions/cache/savepair (GitHub's own cache service, not local disk) around both the rootnpm ciandreview-enrichment's separatenpm ci(it isn't an npm workspace member, so it has its ownpackage-lock.jsonand needs its own cache entry). The install step is skipped entirely when the restore is an exact hit; the save step is placed immediately after a successful install (not as an automatic post-job hook), so a job that fails installing never reaches it — a broken/partialnode_modulescan never get written to the cache for a future run to inherit.runs-onsplit): the self-hosted runner's Docker image and GitHub'subuntu-latestare not guaranteed binary-compatible for native modules (sharp,workerd,fsevents,@sentry/cliall compile platform-specific binaries), so crossing them could load an incompatible binary. Fork PRs get read-only cache tokens (documentedactions/cachebehavior) and can never write a "fork"-keyed entry — they keep doing a fullnpm ciexactly as before, no regression on that no-retry population..nvmrc) with no lockfile change would still hit and silently reusenode_moduleswhose native addons were compiled against the OLD Node's ABI. Both keys now also hash.nvmrc.test/unit/ci-dependency-cache.test.tsto pin the restore/skip/save wiring for both install paths, including the.nvmrcfix and that REES's build/test step still runs unconditionally (independent of whether install ran or was skipped this run).Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.md.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coverage— full unsharded run green on Node 22.23.1 (matching CI's pinned.nvmrc): 314 passed / 2 skipped, 5927 tests passed, 0 failures.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateci.yml's structure (workflow-runner-labels,codecov-policy,change-guardrail,agent-guardrail-paths,path-matchers) all pass unchanged.node_moduleslayout empirically: a fresh rootnpm cialso createsapps/gittensory-ui/node_modules(a small, gitignored, npm-workspaces hoisting-conflict artifact) — both paths are included in the root cache entry.actions/cache's documented restore/save split semantics against its own README before implementing (not from memory):cache-primary-keyoutput feedingsave'skeyinput is the officially documented pattern; fork-PR read-only cache tokens are documented behavior, not an assumption..nvmrcin the cache key), which is fixed and now has a dedicated assertion in the new test.test:coveragerun (selfhost-sqlite-queue.test.ts, unrelated to this diff) while the machine was heavily loaded from parallel background work; reran that file in isolation (52/52 passed) and reran the full suite cleanly (0 failures) to confirm it was local system-load flakiness, not a regression.If any required check was skipped, explain why:
Safety
Notes
validate-code's remaining cost after both merged)..nvmrc+ lockfile combination will still be a fullnpm ci(cache miss, then save) — the skip is only observable on a second run against the same combination.--cachewas technically verified (~4x local speedup withcache-strategy: content) but the net win is marginal-to-negative once fork-PR risk andactions/cache's own overhead are weighed in.