fix(platform): resolve pnpm via corepack when npm_execpath is absent (#2438)#3038
fix(platform): resolve pnpm via corepack when npm_execpath is absent (#2438)#3038leno23 wants to merge 1 commit into
Conversation
Nested tools-dev daemon/desktop builds no longer assume a global pnpm binary on PATH. Fall back to `corepack pnpm …` on Windows and POSIX so the documented Corepack startup flow works end-to-end.
lefarcen
added
size/XS
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33f99b1a18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (process.platform === "win32") { | ||
| return buildCmdShimInvocation("pnpm", args, env); | ||
| return buildCmdShimInvocation("corepack", ["pnpm", ...args], env); | ||
| } | ||
| return { args, command: "pnpm" }; | ||
| return { args: ["pnpm", ...args], command: "corepack" }; |
There was a problem hiding this comment.
When npm_execpath is absent, this now unconditionally executes corepack pnpm instead of pnpm. In environments where Corepack cannot hydrate its cache (for example fresh machines behind a restrictive proxy/offline) this path fails even if pnpm is already installed globally, which blocks nested tools-dev/tools-pack package-manager calls that previously succeeded. The regression is specifically on the missing-npm_execpath fallback path, so it needs a direct-pnpm fallback when Corepack is unavailable.
Useful? React with 👍 / 👎.
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hey @leno23, thanks for tying this fix back to #2438 — the Corepack-only Windows fallback is easy to understand from the summary. One quick PR-body cleanup before pool review: could you update ## Surface area to the template checklist and tick the applicable box (likely Default behavior change) instead of listing the touched files only?
Related: #2144 is open in the same package-manager invocation area, and #2445 was an earlier attempt for #2438; useful context for keeping the scope aligned.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@leno23 I reviewed the changed packages/platform fallback and the matching unit updates, and the no-npm_execpath path now consistently resolves nested package-manager invocations through corepack pnpm on both POSIX and Windows instead of assuming a global pnpm binary. Thanks for tightening up this Windows Corepack startup path — nice, focused fix. 🙌
3 participants
Fixes #2438
Summary
When
npm_execpathis not set (common on Windows Corepack-only setups), nested package-manager invocations fromtools-devnow runcorepack pnpm …instead of assuming a globalpnpmbinary is on PATH.Surface area
packages/platform/src/index.ts—createPackageManagerInvocationfallbackpackages/platform/tests/index.test.ts— updated fallback expectationsValidation
Made with Cursor