feat(env)!: replace Corepack with package manager shims - #2391
feat(env)!: replace Corepack with package manager shims#2391liangmiQwQ wants to merge 3 commits into
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
|
@codex review |
![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: 86e420d5a6
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 706d8e318c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c08fd4bbd
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b07212bb3
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7e22c2808
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbb9b57ba3
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
vite-plus/crates/vp_pm_cli/src/package_manager.rs
Lines 905 to 910 in 7ddce54
Verify Bun's declared package-manager hash
When a project uses an integrity-pinned declaration such as packageManager: "bun@1.2.3+sha512...", this branch discards expected_hash, so even an incorrect pin succeeds and the downloaded native binary runs. Verifying the platform tarball against dist.integrity fetched from the same registry does not enforce the user-declared hash; verify the Bun package artifact named by the declaration, as the native pnpm path does.
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e45fa4a9bd
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd92acbfd0
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92424fbef1
ℹ️ 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".
|
Cleaned up the review-driven follow-ups in d0cc793: removed alias repair, env which expansion, mismatched-manager fallback, npm ownership tracking, downgrade cleanup, and transactional shim backup/restore. Those behaviors are either deferred product work, unsupported damaged-state recovery, or conflict with this PRs breaking-change boundary. Retained only direct normal-path fixes: cached unpinned managers, native Bun/pnpm runtime handling, Node override/error propagation, complete installer disclosure, and an explicit error for vp install -g corepack. The cleanup commit is +91/-855; the full PR is now +452/-1321. |
|
@codex review |
|
Final follow-up pushed:
No further Codex review requested. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
fengmk2
added
the
preview-build
| 'vpr', | ||
| ]; | ||
|
|
||
| for (const shim of ['vp', 'node', 'npm', 'npx', 'corepack', 'vpx', 'vpr']) { |
There was a problem hiding this comment.
The reason we previously chose corepack was because we were unsure what side effects would result from defaulting to create shims for pnpm, yarn, and bun.
This is because it would prevent users from installing pnpm or bun themselves once they have installed vp.
We may need to make some modifications in install.sh to prompt users whether they want vp to manage pnpm, yarn, bun, etc., rather than defaulting to overriding their installations.
There was a problem hiding this comment.
We may need to make some modifications in install.sh to prompt users whether they want vp to manage pnpm, yarn, bun, etc., rather than defaulting to overriding their installations.
I thought about that, so now vp env off can control the pnpm binary status like Node.js.
I also plan to introduce partial control like vp env off pm vp env on node in the second PR.
There was a problem hiding this comment.
The reason we previously chose corepack was because we were unsure what side effects would result from defaulting to create shims for pnpm, yarn, and bun. This is because it would prevent users from installing pnpm or bun themselves once they have installed vp.
The first point, yes, but in order to divide the responsibilities, and reduce mixed up logic, the architecture requires Vite+ offers full control underlying package manager.
And since vp env will manage package managers soon, there are no problems if they want to install their own package managers.
|
@liangmiQwQ You can continue to complete and submit the remaining 2 PRs, and I will continue the review. Once all 3 PRs have no special issues, I will merge them in one go. |
@fengmk2 Should I just submit them towards main? I'm worried that will make the second PR includes the first PR's diff, as well as the third one. |
c7e51be to
c3c58d0
Compare
Two fixes to `main`, both found by the first real runs of the publishing workflow. It could not be exercised before merge, because `workflow_run` only fires for workflow files already on the default branch. **First, the good news: the design works.** PR #2328 published end to end through the new path with an OIDC token, no admin token involved. `authorize`, `Pkg Preview`, and the sticky comment all succeeded, and `commit.a7180fa85c06fad48` is on the bridge: ``` commit.a7180fa85c06fad48 | pr: .../pull/2328 | at: 2026-08-10T07:35:31.617Z ``` ## 1. Fork PRs could not be resolved at all #2391 (from `liangmiQwQ`) failed in `authorize` with `no open PR of voidzero-dev/vite-plus has head c7e51be…` while that PR was open with exactly that head. `listPullRequestsAssociatedWithCommit` returns **empty** for a fork PR's head commit. Confirmed against the live API: | commit | result | | --- | --- | | #2387 head (same-repo) | returns `#2387` | | #2391 head (fork) | **empty** | So it worked for every case reachable before merge and failed for the only case this feature exists for. `workflow_run.pull_requests` is empty for forks too, which is what sent me to the commit endpoint originally — I swapped one fork-blind source for another. Now resolves via `pulls?state=open&head=<head_owner>:<head_branch>`, both GitHub-signed payload fields. The head-sha match is a separate step so the message distinguishes "no such PR" from "the PR moved on": ``` fork PR 2391 (real failure) -> OK: #2391 labeled=true fork=true stale head -> FAIL: PR #2391 now at c7e51be, built 0000000 no such branch -> FAIL: no open PR from liangmiQwQ:does-not-exist ``` I re-checked the rest of the publishing workflow for the same blind spot. Everything else keys off the PR number or the run id, which are base-repo objects and fork-safe: the post-approval `pulls.get` re-check returns correct state, head and labels for #2391, and the artifact download and the `listWorkflowRunArtifacts` precondition both see that run's 148MB `bridge-packages`. ## 2. The Docker gha cache broke the image push ``` #14 exporting to GitHub Actions Cache #14 ERROR: error writing layer blob: failed to reserve cache #13 exporting to image ... CANCELED ``` The cache export is fatal to the build, so it cancelled the push. I added this in the cleanup pass; it broke the job it was meant to speed up, and #2328's npm preview published while its Docker image did not. Reverted rather than repaired. Making it work needs `actions: write` on the one job that installs and executes the preview package, which is the job SR-5 says to keep unprivileged, and this was the only `type=gha` usage in the repo so there was no working precedent. It was saving 60-90s of apt on a path that already waits on a human approval measured in minutes to days. ## 3. Terminology "Trusted leg" and "build leg" were my own coinage and meant nothing to a reader who was not in the design conversation. The two workflows are now described as **the build workflow** and **the publishing workflow**, and where trust was the point the property is stated rather than encoded in a name. This also surfaced something worth fixing later: `publish-preview.yml` is named "Publish preview build" and no longer publishes anything. Renaming it is the real fix, but the publishing workflow matches it by `name:`, so that has to be a coordinated change. The header says so outright for now. The same terminology fix for the RFC and bridge docs is voidzero-dev/pkg-pr-registry-bridge#93, which also corrects SR-1 for the fork-blind endpoint above. ## After merging Re-label #2391 to get the first genuine fork preview.
|
@liangmiQwQ You can have the agent handle it, which should be able to do this kind of thing well. Even without stacked PRs, it can create PRs by branch. |
c3c58d0 to
5c2cae7
Compare
2 participants
The first part of #2375.
Related to #858, #1309.
Breaking Change: users can no longer access
corepackbinary, nor can they runvp install -g pnpm(alsocorepack,bun,yarn) as they did before. The missing feature, likecorepack useand install specific-pnpmversion as failback, will be shipped in the second PR (vp envenhancement)This PR removes the Corepack integration from the global CLI and environment setup flow.
vp env setupnow owns shims for npm/npx, pnpm/pnpx, Yarn/yarnpkg, and Bun/bunx, resolving project-declared versions while keeping npm bundled with Node.js.Refreshing the environment removes legacy managed package-manager installs and stale Corepack shims so old state cannot shadow the new defaults. Installer scripts, docs, and CLI snapshots are updated to match.
Two follow-up layers are staged in the fork and will be submitted sequentially after prior layers merge.
Bun and pnpm 12 native-binary support already exists on main (#2151, #2289). This PR reuses it and adds coverage for reaching it through the new default shims.
🤖 Generated with Codex