fix(cache): correct the default store-directory table and adopt the effected sweep surfaces - #209
Conversation
…ffected sweep surfaces The default cache-directory table had three wrong cells, so a macOS or Windows job archived directories its package manager never writes to. That is a cold cache rather than a failure, which is why it survived a release. It now comes from PackageManagerCache, which cites an authority per row: pnpm's macOS store, both yarn rows, and bun on Windows all move. Four hand-rolled helpers give way to the upstream surfaces that replace them, with no change to the cache keys a run produces. GitHubContext.branch takes the branch fallback chain, CacheKey.digest both short-digest sites, ChildEnv the child PATH and the Windows shell rule, and filenamesFor the lockfile names. The workspace-config patterns and deno's rows stay local, since neither is in the kit's vocabulary. One behavior change falls out of GitHubContext.branch: a tag push now keys under the tag rather than sharing the branchless bucket with every other tag. The cross-branch restore rung means a cold tag still restores what its branch saved. Closes #207 Signed-off-by: C. Spencer Beggs <spencer@savvyweb.systems>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesCache and runtime alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request updates the action’s dependency-caching behavior and internal “kit” integrations by adopting newer @effected/* surfaces, while fixing incorrect default package-manager cache-directory paths that previously caused cold caches on some platforms.
Changes:
- Fix default package-manager cache directory detection by delegating to
@effected/npm’sPackageManagerCache.defaultDirectoryand expanding platform-specific test coverage. - Adopt new upstream helpers (
GitHubContext.branch,CacheKey.digest,ChildEnv.*,filenamesFor) to replace local hand-rolled logic. - Update dependencies and document the new adopted surfaces, including a changeset entry.
Reviewed changes
Copilot reviewed 9 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/steps/restore-cache.ts | Switches GitHub context reads to a single env.github decode (workspace + branch), including tag-keying behavior. |
| src/steps/install-dependencies.ts | Replaces local PATH/shell logic with ChildEnv.prependPath and ChildEnv.needsShell. |
| src/steps/cache-config.ts | Uses PackageManagerCache.defaultDirectory, filenamesFor, and CacheKey.digest to build cache paths and key segments. |
| test/unit/steps/restore-cache.test.ts | Updates env fixtures for GITHUB_REF_NAME and adds a tag-push cache-keying test. |
| test/unit/steps/install-dependencies.test.ts | Adjusts assertions to validate target-platform PATH delimiter behavior. |
| test/unit/steps/cache-config.test.ts | Splits cache-path assertions across Linux/macOS/Windows and validates corrected directories. |
| package.json | Bumps @effected/* and @vitest-agent/plugin versions to adopt post-sweep surfaces. |
| pnpm-lock.yaml | Updates lockfile to reflect the dependency bumps. |
| CLAUDE.md | Updates repository documentation to reflect new imported surfaces and versions. |
| .changeset/olive-parrots-swim.md | Adds a patch changeset documenting the cache-dir fix and refactors. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
| * `ChildEnv.needsShell` decides the launch, and all four managers take its | ||
| * `true` on win32 — bun included, which is this call site's own ruling rather | ||
| * than the kit's. bun is the exception that shows the shape of the rule: |
Code ReviewCurrent Commit: SummaryA tight, correctness-focused change that swaps five hand-rolled surfaces for What I verified
Test coverageExcellent. macOS was previously untested for store paths — which is precisely how the pnpm cell stayed wrong — and it's now pinned separately alongside Linux and Windows. The tag-push case is new and meaningful. One minor note (non-blocking)Copilot flagged the docstring at ValidationAll checks green: Tests, Code Quality, Markdown, Conventional Commits, PR Title Validation. Clean, correct, and well-tested — the docstrings-as-rationale style makes the intent easy to audit. Approving. |
![savvy-web-bot[bot]](https://avatars.githubusercontent.com/in/1855974?s=60&v=4){kind=small}
Adopts the effected post-loop-sweep surfaces (round-7 tail) and fixes the cache-directory correctness bug that came with them.
The cache-dir fix
Three cells of the default store-directory table were wrong, so a macOS or Windows job archived directories its package manager never writes to — a cold cache rather than a failure, which is why it survived a release. The table now comes from
@effected/npm'sPackageManagerCache, which cites an authority per row.~/.local/share/pnpm/store~/Library/pnpm/store~/Library/Caches/Yarn~/.yarn/cache~/.cache/yarn~/.cache/yarn~/.yarn/berry/cache%LocalAppData%\Yarn\Berry\cache%LocalAppData%\bun\install\cache~/.bun/install/cache~/.yarn/cachewas never either yarn's global cache. deno stays local policy — it is out of the kit's vocabulary by scoping.The five adoptions
Each deletes a hand-roll, with no change to the cache keys a run produces:
GitHubContext.headRef+branchreplaces the rawGITHUB_HEAD_REF/GITHUB_REFfallback chain inrestore-cache.ts, folded into the singleenv.githubread that already supplied the workspace. The kit encodes the trap that the runner writesGITHUB_HEAD_REFas the empty string on non-PR events.CacheKey.digestreplaces both hand-rolledsha256(…).slice(0, 8)sites. Concatenating before hashing keeps the segments byte-identical.ChildEnv.prependPath/needsShellreplacepathKeyOf,childEnvandneedsShellininstall-dependencies.ts. The child'sPATHis now joined with the target platform's delimiter rather than the host's.filenamesForsupplies the lockfile names. The workspace-config extras (pnpm-workspace.yaml,.pnpmfile.cjs,.pnp.cjs,.yarn/install-state.gz) and deno's row stay local, per the round-7 ruling. The resulting pattern set is unchanged — this action's table already carriednpm-shrinkwrap.jsonandbun.lockb.PackageManagerCache.defaultDirectory— the table above.One behavior change
A tag push now keys its cache under the tag name. The chain this replaces saw a ref that was not
refs/heads/*and answered"", so every tag in the repository shared the one branchless bucket. Depth 3 ofRESTORE_DEPTHSdrops the branch segment, so a cold tag still restores what the branch it was cut from saved. Pinned by a new test.On the
EADDRINUSEitemAlready fixed in the released code, not carried here. Both listeners in
turbo-cache.test.tsbind port 0 and read the assigned port back underEffect.acquireUseRelease, and no test binds41230— the remaining literals are formatting and state-schema fixtures with no listener behind them. That landed in #206, so 1.3.0 contains it; no test failure appears in the last 60 CI runs.Verification
pnpm typecheckandpnpm buildclean;dist/and.github/actions/local/committedCloses #207
Signed-off-by: C. Spencer Beggs spencer@savvyweb.systems
Correct package-manager cache paths and use shared cache helpers.