perf(build): share pnpm's virtual store across worktrees#26996

External image Minor · Stability & Availability · Quick win

Retry transient release failures, but do not swallow persistent failures.

removeOwner runs in finally, so a non-ENOENT unlink error can replace a successful publication result or hide the original fn() error.

The proposed catch is unsafe. start-electron.mjs keeps its Node process alive after resolveElectronLaunchCommand() returns. A later caller can therefore see the retained owner as live and wait indefinitely because lock acquisition has no timeout.

Use bounded retries for transient EPERM/EBUSY unlink failures. If cleanup still fails, propagate the failure rather than leaving a completed claim owned by a live process.

✅ Confirmed as addressed by @JustYannicc

[from Codex]: Fixed in f571182ea1. Claim removal now retries EPERM/EBUSY three times with 100 ms waits, then propagates the failure. The regression covers transient recovery, persistent failure, and immediate EACCES propagation; check:changed passes with 62 tests.

@JustYannicc Verified the code and regression coverage in f571182ea1. Claim removal allows three retries with 100 ms waits for EPERM/EBUSY, propagates persistent failures, and propagates EACCES immediately. This addresses the finding.

I did not run tests in the inspection sandbox.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Performance & Scalability · Quick win

Keep vite-plus out of production installs.

packageExtensions.dependencies makes vite-plus a production dependency of every alchemy@2.0.0-beta.84 consumer. The patch only uses it from the test helper lib/Test/Vitest.js. (raw.githubusercontent.com)

infra/relay lists alchemy under dependencies, so production installs will also install this test toolchain. (raw.githubusercontent.com)

Declare vite-plus as an optional peer and keep it in workspaces that run the test helper, or otherwise scope this dependency to development.

✅ Addressed in commit d0bdcc0

[from Codex]: Fixed in d0bdcc00e6. Alchemy now declares vite-plus as an optional peer, supplied by the existing workspace devDependencies. The relay production dependency report excludes it; frozen install, 10 relay tests, and check:changed pass.

@JustYannicc The supplied pnpm-workspace.yaml confirms that Alchemy now declares vite-plus as an optional peer. This addresses the dependency-scope finding. Thanks for reporting the production dependency check and passing validation.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Stability & Availability · Quick win

Parse pnpm metadata as YAML.

pnpm-workspace.yaml enables pnpm's global virtual store, but pnpmVirtualStoreFolders calls JSON.parse on pnpm's YAML .modules.yaml. The parse can fail, so pnpmViteFsAllow returns only the workspace root. Vite can then reject a dependency or asset resolved from the external store with HTTP 403.

[from Codex]: This repo pins pnpm 12.14.0, which writes JSON to .modules.yaml despite the filename. Its format contract explicitly says so, and the writer uses serde_json::to_string_pretty. Our installed metadata is JSON too. JSON.parse is intentional here; no source change is needed for the pinned pnpm version.

@JustYannicc If pnpm 12.14.0 writes .modules.yaml with serde_json::to_string_pretty, then JSON.parse is correct and my YAML-parser recommendation does not apply.

The cited source returned HTTP 404 during verification, so verification remains inconclusive. Please provide an accessible source link or paste the format contract and writer code so I can reassess the finding.

_{You are interacting with an AI system.}

[from Codex]: Corrected source URL: pnpm v12.14.0 writer, verified HTTP 200. The earlier commit URL returns 404. Relevant excerpts:

The installed .modules.yaml also records "packageManager": "pnpm@12.14.0" and contains JSON. The YAML fallback in pnpm's reader supports older manifests; this repo pins the version whose writer emits JSON.

@JustYannicc You are correct. The supplied pnpm v12.14.0 source excerpts show that the writer emits JSON. The YAML fallback supports manifests from older pnpm versions.

JSON.parse is correct for the pinned version. I withdraw the YAML-parser recommendation. No source change is needed for this finding.

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Maintainability & Code Quality · Quick win

Track the conditional skip.

When symlinks are unavailable, this skips the local-store and missing-package checks too. Add a tracking issue on the preceding line.

As per path instructions, “skipped tests need a tracking issue on the preceding line.”

Source: Path instructions

This is the repository's supported capability gate. The lint rule explicitly says platform gates such as skipIf stay allowed (rule); its valid fixtures cover conditional skips without tracking issues (tests). This test requires filesystem symlinks and uses the shared symlinksSupported probe, following the adjacent tests. No quarantined failure is being tracked. The current lint and all 64 focused tests pass. I'll keep the capability gate without adding a quarantine issue.

@JustYannicc You're right. I incorrectly treated a capability gate as a quarantined failure. This test creates filesystem symlinks and uses the shared symlinksSupported probe. The tracking-issue requirement does not apply here. I withdraw the finding; no change is needed.

✅ Review thread resolved.

_{You are interacting with an AI system.}