fix(init): only detect pnpm approve-builds failures from pnpm-specific output

[Boyeep]

What changed

Tightened vinext init's approve-builds error detection so it only treats install failures as pnpm build-approval issues when the output is actually pnpm-specific.

In practice, this narrows isApproveBuildsError() to match:

• explicit approve-builds output
• ERR_PNPM_*BUILD
• generic ignored build scripts / blocked build scripts text only when pnpm is also present

Why

vinext init was previously too eager here and could misclassify a generic install failure as a pnpm approve-builds case.

That led to the CLI swallowing the real install error and printing pnpm recovery instructions even when the failure was not actually a pnpm build-approval problem.

Impact

• preserves the real install error for non-pnpm or unrelated failures
• keeps the pnpm approve-builds recovery flow working when the output is genuinely pnpm-specific
• improves vinext init failure reporting and recovery guidance

Root cause

The approve-builds matcher was keyed off broad phrases like ignored build scripts without requiring enough pnpm-specific context.

Validation

Ran:

• pnpm test tests/init.test.ts -t "does not classify non-pnpm install failures as approve-builds errors"
• pnpm test tests/init.test.ts -t "approve-builds"
• pnpm test tests/init.test.ts

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2404

npm i https://pkg.pr.new/vinext@2404

commit: 5bb9d3f

[github-actions]

Performance benchmarks

Compared 5bb9d3f against base 31e8743 using alternating same-runner rounds. Next.js was unchanged and skipped.

1 improved · 0 regressed · 5 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	124.0 KB	124.0 KB	⚫ +0.0%
Client entry size (gzip)	vinext	118.8 KB	118.8 KB	⚫ +0.0%
Dev server cold start	vinext	2.56 s	2.52 s	🟢 -1.5%
Production build time	vinext	3.03 s	3.06 s	⚫ +1.2%
RSC entry closure size (gzip)	vinext	95.2 KB	95.2 KB	⚫ -0.0%
Server bundle size (gzip)	vinext	161.5 KB	161.5 KB	⚫ -0.0%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(init): tighten approve-builds detection

The change to isApproveBuildsError() is correct and a real improvement. Splitting the matcher so that the generic ignored build scripts / blocked build scripts phrases only trigger when pnpm is also present in the output is the right call — those phrases are not pnpm-exclusive, so the previous broad regex could misclassify unrelated install output. The approve-builds and ERR_PNPM_*BUILD branches stay unconditional, which is appropriate since both are unambiguously pnpm-specific.

I ran vitest run tests/init.test.ts — all 101 tests pass.

Main concern: the change is effectively untested

The PR description's Validation section cites the test "does not classify non-pnpm install failures as approve-builds errors" (tests/init.test.ts:962). That test already exists on main (last touched in #2389) — it is not added by this PR, and it does not exercise this change.

The reason: that test goes through the catch error path, which is gated on the package manager before isApproveBuildsError is consulted:

// init.ts:521
if (pmName !== "pnpm" || !isApproveBuildsError(error)) throw error;

The test's fixture (setupProject(tmpDir, { router: "pages" })) writes no pnpm-lock.yaml, so pmName !== "pnpm" short-circuits to true and the error is rethrown regardless of what isApproveBuildsError returns. That test therefore passes identically against both the old and new regex — it does not protect against regressions in this function.

The behavior this PR actually changes lives in the success path (init.ts:505 and init.ts:519), which calls isApproveBuildsError(installOutput) for any package manager and is not guarded by pmName. There is currently no test covering: a successful install whose output contains ignored build scripts without pnpm → should NOT set dependencyInstallNeedsApproval. Adding such a test would lock in the fix and prevent the broad regex from creeping back.

This is non-blocking (the fix is correct), but I'd recommend adding a targeted test for the success path so the change is genuinely covered.

[ask-bonk]

The logic here is right. Note the call site that this actually affects in practice is the success path (init.ts:505 / init.ts:519), which invokes isApproveBuildsError(installOutput) for any package manager. The catch error path (init.ts:507 / init.ts:521) is gated on pmName === "pnpm" first, so for non-pnpm projects the error is rethrown before this function is even consulted. That's why the cited validation test ("does not classify non-pnpm install failures as approve-builds errors") passes against both old and new code — it doesn't exercise this change. A test on the success path (successful install whose output contains ignored build scripts but no pnpm → dependencyInstallNeedsApproval stays false) would actually cover this fix.

[ask-bonk]

Review posted successfully. Summary of my findings:

Review summary for PR #2404

Verdict: the fix is correct, but effectively untested.

What's good

The change to isApproveBuildsError() is sound. Splitting the matcher so that the generic phrases ignored build scripts / blocked build scripts only match when pnpm is also present (while keeping approve-builds and ERR_PNPM_*BUILD unconditional) correctly narrows detection to genuinely pnpm-specific output. All 101 tests in tests/init.test.ts pass.

Key issue raised

The PR description's "Validation" section cites the test "does not classify non-pnpm install failures as approve-builds errors" (tests/init.test.ts:962), but:

1. That test already exists on main (from perf(routing): cache app-route-graph directory reads — 5000-route build −32% #2389) — the PR adds no test of its own (it only modifies init.ts, +3/-2).
2. That test does not exercise the change. It hits the catch error path, which is gated on pmName === "pnpm" before isApproveBuildsError is consulted (init.ts:521). With no pnpm-lock.yaml in the fixture, pmName !== "pnpm" short-circuits and the error rethrows regardless of the regex — so the test passes identically against old and new code.

The behavior the PR actually changes is the success path (init.ts:505/519), which calls isApproveBuildsError(installOutput) for any package manager and is unguarded by pmName. That path has no test. I recommended adding one: a successful install whose output contains ignored build scripts but not pnpm should leave dependencyInstallNeedsApproval false.

This is non-blocking since the code change is correct, but the validation claim is misleading and the fix lacks real regression coverage.

github run