Skip to content

ci: seed pnpm cache after lockfile refresh - #1201

Merged
kkroo merged 5 commits into
masterfrom
codex/refresh-lockfile-cache-seed
Aug 11, 2026
Merged

ci: seed pnpm cache after lockfile refresh#1201
kkroo merged 5 commits into
masterfrom
codex/refresh-lockfile-cache-seed

Conversation

@kkroo

@kkroo kkroo commented Aug 9, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the application used to coordinate and operate work by AI agents.
  • Pull-request jobs install the same PNPM dependency graph across many ephemeral ARC runners.
  • actions/setup-node restores a lockfile-keyed PNPM store from the on-prem Actions cache.
  • Refresh Lockfile enabled that cache but ran only pnpm install --lockfile-only, so its post-job hook published an empty default-branch store for each new lock hash.
  • Downstream jobs reported a cache hit for 186 bytes, then downloaded all 1,324 packages with reused 0.
  • This pull request refreshes the lockfile first, restores the compatible key explicitly, populates the store with pnpm fetch, and saves only after fetch succeeds.
  • The benefit is a complete default-branch cache that PRs can actually reuse without changing their existing setup-node integration.

Linked Issues or Issue Description

What happened?

Refresh Lockfile run 31277773916 created a 186-byte cache entry for a new lock hash. Exact-run logs from a downstream PR restored that entry successfully, but pnpm install still 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

  1. Push a dependency manifest change to master that produces a new pnpm-lock.yaml hash.
  2. Let Refresh Lockfile complete with cache: pnpm and pnpm install --lockfile-only.
  3. Inspect the new node-cache-Linux-x64-pnpm-* entry and run a PR job against that lockfile.
  4. Observe a tiny cache hit followed by reused 0 and a full package download.

Paperclip version or commit

b9f9dd2a5b60c4f13ec2d6c78433762b2c446767

Deployment mode

GitHub Actions on ephemeral Kubernetes ARC runners using the on-prem Falcon cache service.

What Changed

  • Remove implicit PNPM caching from Refresh Lockfile's setup-node step.
  • Resolve the PNPM store path and Node architecture after refreshing the lockfile.
  • Restore and save with setup-node's exact node-cache-<OS>-<arch>-pnpm-<lock hash> key shape.
  • Populate the store with pnpm fetch --frozen-lockfile before the explicit save action can run.
  • Add regression coverage for key compatibility, success gating, and step order.

Verification

  • node --test scripts/__tests__/ci-cache-routing.test.mjs
  • Ruby YAML parse of .github/workflows/refresh-lockfile.yml
  • git diff --check

After 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

  • The first Refresh Lockfile run for a new hash now downloads the dependency graph once, adding time to that low-frequency workflow.
  • Cache entries are immutable. Any poisoned entry for the current hash must be deleted before merging this PR so the populated store can claim the key.
  • The save step is skipped on an exact hit and is gated by success(), so failed fetches cannot publish partial stores.

Model Used

  • OpenAI Codex, GPT-5, with tool use, GitHub Actions inspection, Kubernetes inspection, and extended reasoning.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this CI repair does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and found none open
  • I have described the issue in-PR following the bug template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • This change does not affect the UI
  • The workflow and regression test document the new cache contract
  • I have considered and documented the risks above
  • All Paperclip CI gates are green
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

@cursor

cursor Bot commented Aug 9, 2026

Copy link
Copy Markdown

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.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Safe to merge.

@allyblockcast allyblockcast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Safe to merge.

@kkroo
kkroo force-pushed the codex/refresh-lockfile-cache-seed branch from 12c9535 to ab31b28 Compare August 9, 2026 11:30

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Safe to merge after the pending CI checks complete.

@allyblockcast allyblockcast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Safe to merge after the pending CI checks complete.

kkroo and others added 2 commits August 9, 2026 04:35
@kkroo
kkroo enabled auto-merge August 9, 2026 11:48

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Safe to merge after required CI completes.

@allyblockcast allyblockcast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Safe to merge after required CI completes.

@kkroo
kkroo disabled auto-merge August 9, 2026 11:54
@kkroo

kkroo commented Aug 9, 2026

Copy link
Copy Markdown
Author

@ally please provide formal App and User reviews for exact head b0546af.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Merge when required checks complete.

@allyblockcast allyblockcast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Merge when required checks complete.

@kkroo
kkroo added this pull request to the merge queue Aug 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 10, 2026
@kkroo
kkroo added this pull request to the merge queue Aug 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 10, 2026
@kkroo
kkroo enabled auto-merge August 10, 2026 11:02

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Safe to merge after the pending CI checks complete.

@kkroo
kkroo added this pull request to the merge queue Aug 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 10, 2026
@kkroo
kkroo added this pull request to the merge queue Aug 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 11, 2026
@kkroo

kkroo commented Aug 11, 2026

Copy link
Copy Markdown
Author

Merge-group run 31462473806 exposed two current-master test failures unrelated to this PR. Fix is in #1298; keeping #1201 out of the queue until #1298 lands, then this PR will be requeued against the corrected master.

@kkroo
kkroo added this pull request to the merge queue Aug 11, 2026
@kkroo
kkroo removed this pull request from the merge queue due to a manual request Aug 11, 2026
@kkroo
kkroo merged commit ede94ef into master Aug 11, 2026
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants