feat(pm): add vp pm approve-builds subcommand#1662
Merged
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
fengmk2
force-pushed
the
feat/pm-approve-builds
branch
from
06be8c5 to
233ec80
Compare
fengmk2
commented
May 24, 2026
Member
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
fengmk2
force-pushed
the
feat/pm-approve-builds
branch
from
ead5765 to
7d975a4
Compare
Member
Author
|
@cursor review |
Adds a unified `vp pm approve-builds` command that mirrors `pnpm approve-builds` and adapts to `bun pm trust`, with informative warn-and-noop fallbacks for npm and yarn. Surface (intentionally tight, matches pnpm's documented flags): vp pm approve-builds # interactive (pnpm) vp pm approve-builds esbuild # approve named packages vp pm approve-builds esbuild !core-js # pnpm >= 11.0.0 (deny syntax) vp pm approve-builds --all # pnpm >= 10.32.0 / bun vp pm approve-builds -- <raw args> # forward to underlying PM Cross-PM behavior: - pnpm: pass-through. Version-gates `--all` on >= 10.32.0 and `!pkg` deny syntax on >= 11.0.0 (per pnpm PR #11030); prerelease versions compared against the lowest prerelease floor (`10.32.0-0` / `11.0.0-0`) so RCs are accepted. - bun: `bun pm trust [--all] [pkgs...]`. `!pkg` tokens emit a warn and are filtered (bun has no denylist model). When only deny tokens are given, the warn alone is enough context — no redundant note. - npm: warn and exit 0, pointing at `ignore-scripts=true` in `.npmrc`. - yarn: warn and exit 0. Yarn 1 (Classic) gets an npm-style hint (lifecycle scripts run by default); yarn Berry gets `dependenciesMeta.<pkg>.built: true` advice. Safety: - `--all` and positional packages are mutually exclusive at the clap layer, preventing a silent override where `--all` would otherwise drop denylisted packages on bun. - Version-gate failures render via `Error::UserMessage` (no harsh `error:` prefix). Coverage: - 23 Rust unit tests (resolver, version gates, prereleases, deny gate, yarn 1 vs Berry, pass-through args, strict unparseable-version) - 8 clap parse tests (conflicts, lone-flag, lone-packages, pass-through capture, deny conflict) - 5 snap fixtures: local pnpm10/npm/yarn + global bun/pnpm10-old - RFC at `rfcs/approve-builds-command.md`
`crate-ci/typos` flagged "Unparseable"/"unparseable" as misspellings. The accepted spelling is "Unparsable"/"unparsable". Renames the comment, the test name, and the assertion message.
Addresses PR review: the yarn v1 warning prescribed `ignore-scripts true` (space) but npm's `.npmrc` syntax requires `key=value`. Matches the npm branch's existing wording.
Round-2 review surfaced 3 high-severity issues plus several coverage/UX gaps. Fixes: HIGH severity - Local CLI (NAPI binding) lost the `Error::UserMessage` discriminator introduced in the round-1 friendly-error fix — every error variant was being wrapped in `Error::Anyhow` and printed with the harsh `error:` prefix. Now `execute_pm_command` intercepts UserMessage, prints it via `output::raw_stderr` (no prefix), and returns exit 1. Matches the global CLI's `is_user_message()` behavior. - `vp pm approve-builds --all -- esbuild` bypassed clap's `conflicts_with = "packages"` because the positional went to `pass_through_args` instead — the resolver then emitted `pnpm approve-builds --all esbuild`, which pnpm rejects with `ERR_PNPM_APPROVE_BUILDS_ALL_WITH_ARGS`. Added an up-front runtime guard that rejects `--all` combined with any non-flag pass-through token. Flag-only pass-through (`--silent`, `--loglevel=warn`) still works alongside `--all`. - `pass_through_args` were silently dropped on every no-op return path (npm, yarn, bun deny-only, bun no-args). Now each of those paths emits a `warn` listing the dropped args. UX / surface - Restored `ApproveBuilds` to the `needs_project` set in the handler. The round-1 reasoning (so educational messages can fire outside a project) didn't pan out: in an empty dir vp fell back to a synthetic npm `PackageManager` and the user got the npm `ignore-scripts=true` lecture regardless of their actual stack. approve-builds is fundamentally project-scoped; show `No package.json found.` instead, matching prune/list/audit. Coverage - bun snap fixture: added missing `"ignoredPlatforms": ["win32"]` to match the sibling bun fixtures. - Snap-test redaction (`utils.ts`): normalize the bun-style build hash (`v<semver> (af24e281)` → `v<semver> (<hash>)`) so the snap doesn't drift on every bun rebuild. - Added `command-pm-approve-builds-yarn4` snap fixture covering the yarn Berry branch's distinct `dependenciesMeta.built` warn text (previously only yarn v1 was snap-covered). - Added Rust unit tests: yarn 1.x prerelease (`1.22.22-canary`) still takes the v1 branch; deny-syntax gate rejects unparsable versions; `--all` + positional via `--` rejected at the resolver; `--all` + flag-only pass-through still accepted; npm no-op warns about dropped pass-through args. Tests: vite_install 28/28, vite_pm_cli 5/5, vp check --fix clean, all 6 snap fixtures regenerated.
Per PR review, switch the version-gate helpers from manual
`Version >= Version` comparison to `node_semver::Range` parsing with
the readable `>=10.32.0` / `>=11.0.0` expressions.
Behavior change: npm semver convention now applies — prereleases of any
version do NOT satisfy a release-version range by default. So
`10.32.0-rc.0` and `11.0.0-rc.0` no longer satisfy the `--all` gate;
similarly for the `!pkg` deny gate against `>=11.0.0`. Users on a pnpm
prerelease must bump to the corresponding release before relying on
`--all` or `!pkg`.
Tests flipped accordingly:
- `pnpm_all_accepts_v10_32_prerelease` → `pnpm_all_rejects_v10_32_prerelease`
- `pnpm_all_accepts_v11_prerelease` → `pnpm_all_rejects_v11_prerelease`
- `pnpm_deny_accepts_v11_prerelease` → `pnpm_deny_rejects_v11_prerelease`
Adds `node-semver = { workspace = true }` to `vite_install/Cargo.toml`.
The workspace already declares the dep (v2.2.0).
Tests: 28/28 still passing.
…ixture CI failure had two root causes: 1. The new bun build-hash redaction in packages/tools/src/utils.ts (added in `fix(pm): address PR review findings`) rewrote `v<semver> (af24e281)` → `v<semver> (<hash>)`. Six pre-existing bun fixtures still carried the literal hash and now diff against the redactor output. Regenerated: - command-add-bun - command-list-bun - command-outdated-bun - command-remove-bun - command-update-bun - command-why-bun 2. The local-CLI binding UserMessage intercept (round-2 fix #1) renders `Error::UserMessage` without the `error:` prefix, matching the global CLI. `command-pm-no-package-json` previously captured `error: No package.json found.` from the local CLI; now correctly captures `No package.json found.` (cleaner, matches global behavior). Also adds command-pm-approve-builds-pnpm11 snap fixture covering the green path for pnpm v11's `!pkg` deny syntax (round-3 review coverage gap finding).
fengmk2
force-pushed
the
feat/pm-approve-builds
branch
from
7d975a4 to
d41fa92
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7d975a4. Configure here.
Three new test cases for the `replaceUnstableOutput` change introduced in `fix(pm): address PR review findings`: 1. Positive path — `bun pm trust v<semver> (af24e281)` and other `bun <cmd> v<semver> (<hex>)` banners get the hash normalized to `(<hash>)`. 2. Anchor scoping — unrelated parenthesized hex (e.g. commit SHA fragments in log lines) is NOT redacted; only hashes that follow the `v<semver>` placeholder produced by the prior semver normalization. 3. Case sensitivity — uppercase hex is left alone (bun emits lowercase; case-sensitive regex avoids redacting unrelated uppercase parenthesized text). 29/29 tests pass.
cpojer
approved these changes
May 25, 2026
fengmk2
force-pushed
the
feat/pm-approve-builds
branch
from
69713ac to
851b0d5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
vp pm approve-builds— a unified subcommand for approving dependency lifecycle scripts (install/postinstall). Mirrorspnpm approve-buildsone-to-one, adapts tobun pm trust, and falls back to informative warn-and-noop on npm/yarn.Surface (intentionally tight, matches pnpm's documented flags):
Cross-PM behavior
--allgated on>= 10.32.0,!pkgdeny syntax gated on>= 11.0.0(per pnpm PR #11030). Prereleases (10.32.0-rc.0,11.0.0-beta.1) satisfy viaVersion::parse("<floor>-0")comparison.bun pm trust [--all] [pkgs...].!pkgtokens emit a warn and are filtered (bun has no denylist model). When only deny tokens are given, the warn alone is enough — no redundant note.ignore-scripts=truein.npmrc.dependenciesMeta.<pkg>.built: trueadvice. Per yarn docs,enableScriptsdefaults tofalsein Berry.Safety
--alland positional packages are mutually exclusive at the clap layer (conflicts_with = "packages"). This prevents a silent override where--all !core-json bun would warn "Skipping: core-js" thenbun pm trust --alleverything anyway.Error::UserMessage(no harsherror:prefix).ApproveBuildsis not in theneeds_projectallowlist, so the npm/yarn/bun educational messages can fire outside a project directory.Files
crates/vite_install/src/commands/approve_builds.rs(new),crates/vite_install/src/commands/mod.rs(export),crates/vite_pm_cli/src/cli.rs(variant + parse tests),crates/vite_pm_cli/src/handlers.rs(dispatch + error mapping)rfcs/approve-builds-command.mdcommand-pm-approve-builds-{pnpm10,npm,yarn}/command-pm-approve-builds-{bun,pnpm10-old}/(new)snap-tests-global/cli-helper-message/snap.txt(for the new subcommand entry)Test plan
cargo test -p vite_install --lib approve_builds→ 23/23cargo test -p vite_pm_cli --lib approve_builds→ 8/8vp check --fix→ 0 warnings/errorspnpm -F vite-plus snap-test-local approve-builds→ 3/3 passpnpm -F vite-plus snap-test-global approve-builds→ 2/2 passNote
Low Risk
New PM subcommand with pass-through to native tools; npm/yarn are no-ops and pnpm gates limit misuse. Touches CLI error rendering only for this path.
Overview
Introduces
vp pm approve-buildsso one CLI can approve dependency install/postinstall scripts: pnpm getspnpm approve-builds(including!pkgon ≥11 and--allon ≥10.32.0), bun getsbun pm trustwith!pkgfiltered and warned, and npm/yarn print guidance and exit 0 without shelling out.The resolver uses
node-semverfor pnpm capability checks, rejects--allplus positional packages (including via--), and maps version-gate failures toUserMessageso local and global CLIs show friendly errors.ApproveBuildsis wired throughvite_pm_cli(clap variant, handler, project-scoped PM build) and the local CLI binding handlesUserMessagelike the global CLI.Adds an RFC, per-PM snap tests, updates
vp pm -h, normalizes bun banner hashes in snapshots, and drops theerror:prefix on missingpackage.jsonmessages in one snap fixture.Reviewed by Cursor Bugbot for commit 7d975a4. Configure here.