ci: seed pnpm cache after lockfile refresh - #1201
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 12c9535
Looks good. The workflow refreshes the lockfile before deriving the cache key, restores that exact PNPM store, fetches the resolved dependencies, and saves only on a miss. The focused regression test validates the key, ordering, and conditional save.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Cache population is explicit and tied to the refreshed lockfile hash.
- The regression test protects the setup-node-compatible cache key and sequencing.
Recommended Action
- Safe to merge.
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 12c9535
Looks good. The workflow refreshes the lockfile before deriving the cache key, restores that exact PNPM store, fetches the resolved dependencies, and saves only on a miss. The focused regression test validates the key, ordering, and conditional save.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Cache population is explicit and tied to the refreshed lockfile hash.
- The regression test protects the setup-node-compatible cache key and sequencing.
Recommended Action
- Safe to merge.
12c9535 to
ab31b28
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ab31b28
Looks good. The refresh job derives the cache key after the lockfile update, restores the same store path and key format used by setup-node, fetches the resolved package tarballs, and saves only after a successful cache miss. The focused routing test covers the key, cache-path resolution, success condition, and step ordering.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The explicit restore/save flow prevents the lockfile-only install from publishing an empty store.
- The cache key remains compatible with dependent PR jobs using setup-node's PNPM cache.
Recommended Action
- Safe to merge after the pending CI checks complete.
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ab31b28
Looks good. The refresh job derives the cache key after the lockfile update, restores the same store path and key format used by setup-node, fetches the resolved package tarballs, and saves only after a successful cache miss. The focused routing test covers the key, cache-path resolution, success condition, and step ordering.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The explicit restore/save flow prevents the lockfile-only install from publishing an empty store.
- The cache key remains compatible with dependent PR jobs using setup-node's PNPM cache.
Recommended Action
- Safe to merge after the pending CI checks complete.
…routing [codex] ci: route builds to stable BuildKit cache owners
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b0546af
Looks good. The refresh workflow computes its cache key after refreshing the lockfile, restores and populates the exact pnpm store used by setup-node, and saves only cache misses. Docker workflows now select a stable, probe-verified BuildKit ordinal with a bounded service fallback.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The cache key and store path match setup-node's pnpm implementation.
- The focused routing test covers cache ordering, key symmetry, endpoint selection, and fallback wiring.
Recommended Action
- Safe to merge after required CI completes.
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b0546af
Looks good. The refresh workflow computes its cache key after refreshing the lockfile, restores and populates the exact pnpm store used by setup-node, and saves only cache misses. Docker workflows now select a stable, probe-verified BuildKit ordinal with a bounded service fallback.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The cache key and store path match setup-node's pnpm implementation.
- The focused routing test covers cache ordering, key symmetry, endpoint selection, and fallback wiring.
Recommended Action
- Safe to merge after required CI completes.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d9c3356
Looks good. The lockfile refresh now seeds a store cache keyed to the refreshed lockfile, and BuildKit routing retains a bounded service fallback.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Cache restore, fetch, and save are ordered after the lockfile refresh and use the -compatible key shape.
- BuildKit selection validates the ordinal and falls back to the service endpoint when the preferred pod is unavailable.
- Targeted workflow assertions cover the new cache and endpoint-routing behavior.
Recommended Action
- Merge when required checks complete.
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d9c3356
Looks good. The lockfile refresh now seeds a store cache keyed to the refreshed lockfile, and BuildKit routing retains a bounded service fallback.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Cache restore, fetch, and save are ordered after the lockfile refresh and use the -compatible key shape.
- BuildKit selection validates the ordinal and falls back to the service endpoint when the preferred pod is unavailable.
- Targeted workflow assertions cover the new cache and endpoint-routing behavior.
Recommended Action
- Merge when required checks complete.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2ad9ef1
Looks good. The refreshed-lockfile job derives the same PNPM store key that setup-node consumers use, fills that store before saving, and cannot save after a failed fetch. The BuildKit changes use a bounded probe with a service fallback before selecting the remote builder.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The explicit restore/fetch/save sequence prevents publishing the empty store created by a lockfile-only install.
- The cache path, architecture, and key are symmetric between restore and save, with focused regression coverage.
- Per-ordinal BuildKit routing preserves a verified fallback when the preferred cache owner is unavailable.
Recommended Action
- Safe to merge after the pending CI checks complete.
Thinking Path
Linked Issues or Issue Description
What happened?
Refresh Lockfile run
31277773916created a 186-byte cache entry for a new lock hash. Exact-run logs from a downstream PR restored that entry successfully, butpnpm installstill downloaded all 1,324 packages and reused none.The Falcon cache service itself is healthy and backed by Ceph RGW. The defect is the producer workflow: setup-node's implicit post-job save runs after a lockfile-only install, before package tarballs exist in the PNPM store.
Expected behavior
When Refresh Lockfile creates a new lock hash on
master, it should publish a populated PNPM store under the same key shape used by setup-node. PR jobs using that lock hash should restore real package content instead of an empty hit.Steps to reproduce
masterthat produces a newpnpm-lock.yamlhash.cache: pnpmandpnpm install --lockfile-only.node-cache-Linux-x64-pnpm-*entry and run a PR job against that lockfile.reused 0and a full package download.Paperclip version or commit
b9f9dd2a5b60c4f13ec2d6c78433762b2c446767Deployment mode
GitHub Actions on ephemeral Kubernetes ARC runners using the on-prem Falcon cache service.
What Changed
node-cache-<OS>-<arch>-pnpm-<lock hash>key shape.pnpm fetch --frozen-lockfilebefore the explicit save action can run.Verification
node --test scripts/__tests__/ci-cache-routing.test.mjs.github/workflows/refresh-lockfile.ymlgit diff --checkAfter merge, the automatic master Refresh Lockfile run should create a multi-megabyte cache entry and a subsequent PR install should report nonzero package reuse.
Risks
success(), so failed fetches cannot publish partial stores.Model Used
Checklist