fix(scripts): remove never actually supported npm "--parallel" flag - #8725
Conversation
The `--parallel` flag was a proposal for `npm`, but it wasn't merged in the end. With version 12 unknown flags are now errors instead of warnings, so on that version it blocks running `npm run test` and similar tasks.
8cb33c1 to
f45334c
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: reverse audit — stopped before round 2 by the review time budget. Test Plan (not a blocker): @qwen-code/qwen-code@0.21.7 — no such file or directory.
中文说明
已审查。 建议见行内评论。 未审查:反向审计——评审时间预算不足,未能开始第 2 轮。 Test Plan(非阻断):@qwen-code/qwen-code@0.21.7 — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| [ | ||
| 'cross-env NODE_OPTIONS="--max-old-space-size=3072"', | ||
| 'npm run test:ci --workspaces --if-present --parallel -- --coverage.enabled=false', | ||
| 'npm run test:ci --workspaces --if-present -- --coverage.enabled=false', |
There was a problem hiding this comment.
[Suggestion] Of the three scripts this PR fixes (test, test:ci, test:release), only test:release is pinned by an exact-value assertion; test and test:ci are asserted nowhere (I checked every packageJson.scripts read site). So the exact --parallel regression this PR fixes can silently recur for the two most-used scripts. — Failure scenario: a future change re-adds --parallel (or another unsupported npm flag) to the test or test:ci script in package.json. No test fails, and CI stays green because CI's npm 10.x tolerates unknown flags — exactly why the original flag went unnoticed — while every developer on npm ≥ 12 hits the identical EUNKNOWNCONFIG failure of issue #8721 again at runtime. Probed on both npm versions: 10.9.4 accepts the flag (exit 0), 12.0.2 hard-fails with EUNKNOWNCONFIG, byte-for-byte the issue report. Consider extending the existing pin pattern to cover test and test:ci too, e.g.:
for (const name of ['test', 'test:ci', 'test:release']) {
expect(packageJson.scripts[name]).not.toContain('--parallel');
}— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
I feel like immortalising the original problem (adding a flag to the calls that never actually worked) with a test here is unlikely to be really helpful, and in its suggestged form it wouldn't catch many other potential problems.
There might be better, more general solutions that chip away at this problem (testing with newer npm versions as they come out, testing with a matrix of npm/node versions....) that would catch such regressions or issues better than adding a fragile check.
I have a change like the way it was suggesged above, prepared and happy to push if the above logic doesn't make sense and the test is still preferred.
Verification report — built a real environment and ran this end to endI reviewed this as a maintainer and wanted more than a code read, so I built a real verification environment: three real npm binaries (10.9.7, 11.19.1, 12.0.2) installed side by side and put on Verdict: correct, behaviour-preserving, and merge-ready. I'd merge it. Details and evidence below. 1. The reported failure reproduces exactly, and the fix clears it
The output is byte-identical to the Test Plan in the description, down to 2.
|
| probe | mutation | result |
|---|---|---|
| M1 | re-add --parallel to test:release |
✅ caught — the exact-value pin binds |
| M2 | re-add --parallel to test and test:ci |
❌ blind — see below |
| M3 | inline-expand npm run test:ci in windows-runner-smoke.yml |
✅ caught on this PR, ❌ blind on main |
M3 is worth calling out: narrowing the forbidden substring from npm run test:ci --workspaces --if-present --parallel to npm run test:ci --workspaces --if-present makes that assertion strictly stronger, since the shorter string matches more. The smoke-workflow regression it now catches is one main is blind to today. That edit is a small improvement, not just a mechanical follow-on.
6. Merge readiness against today's main
The PR branched on 8 Aug and both test files it touches have moved substantially on main since, so I merged it and tested the result rather than trusting the mergeable flag:
git mergeis clean — 3 files, +5 −5, no conflicts;- the two touched test files pass on the merged tree: 31/31;
- residual scan finds no
--parallelanywhere except the coreutilssortflag table inpackages/core/src/permissions/shell-semantics.ts, which is unrelated; - full
npm run test:scriptson the merged tree: 4 failed | 1934 passed | 30 skipped. None of the four are caused by this PR. Three of them reproduce on main without the PR in a separate checkout (they assert on unwritable directories, which a root user cannot produce); the fourth,install-script.test.js, passes in a built checkout and only fails here because my verification worktrees were never built, sopackages/audio-capture/distis absent.
On the open thread about pinning test and test:ci
One measured data point rather than an opinion first: with --parallel put back on test and test:ci, the full scripts suite on today's main comes back byte-identical — 4 failed | 1934 passed | 30 skipped, 1968 tests, nothing red. Only test:release is pinned, so the exact regression this PR fixes can silently return to the two most-used scripts.
That said, I agree with @imrehg's reasoning and I don't think it should hold up this PR. A string assertion pins one flag name and would not have caught this particular flag before npm 12 shipped; the thing that actually catches this class is running the suite on a newer npm. If someone wants the belt-and-braces version later, a not.toContain('--parallel') loop over the three scripts is two lines — but it's a separate, optional change, and I'd rather see the npm/node matrix idea pursued instead.
Scope of what I checked
I verified that all three scripts parse and enter vitest under npm 12, not that the entire workspace suite passes there — that's a long run and orthogonal to this change, which only touches argument parsing. Everything else above was executed, not inferred.
中文说明
验证报告 —— 搭建真实环境端到端跑过一遍
我作为维护者审这个 PR,觉得只读代码不够,于是搭了一套真实的验证环境:并排安装三个真实 npm 版本(10.9.7、11.19.1、12.0.2),并用 shim 放进 PATH,保证脚本体里嵌套调用的 npm 也是被测版本;再分别准备了本仓库在 PR base、PR head,以及 PR 8725 合并进今天 main 之后这三种状态的 checkout。
结论:改动正确、行为无变化、可以合并。我支持合入。 证据如下。
1. 报告的失败可以精确复现,改完即消失
在真实 checkout 里、npm 12.0.2 排在 PATH 最前时执行 make test:
| 分支 | 退出码 | 耗时 | 结果 |
|---|---|---|---|
| 不带此 PR 的 main | 1 | 189 ms | EUNKNOWNCONFIG —— 一个测试都没跑就挂 |
| main + PR 8725 | — | 60 s+ | npm 正常解析,vitest 开始跑各 workspace(我在 60 s 时手动终止) |
输出与描述里的 Test Plan 逐字节一致,连 make: *** [Makefile:34: test] Error 1 都一样。三个脚本(test、test:ci、test:release)我都跑了同样的对照:不带此 PR 时三个都在约 185 ms 的参数解析阶段挂掉,带上之后三个都进入 vitest。
2. --parallel 从来就不是真实存在的 npm 参数,也从没起过作用
npm config get parallel 在 10.9.7、11.19.1 和 12.0.2 上都返回 undefined —— 在本仓库能跑的任何 npm 版本上它都不是已定义配置。npm 11 早就打印 npm warn Unknown cli config "--parallel". This will stop working in the next major version of npm.,正是 12 里落地的那次破坏性变更。
为确认删掉它不改变任何行为,我用一个三 workspace 的 fixture 做了对照:每个 workspace 的 test 脚本忙等 1500 ms 并打印自己的起止时间戳,在每个 npm 版本上分别带/不带该 flag 运行:
- 所有能接受该 flag 的组合里,墙钟时间完全一致(约 4.73 s ≈ 3 × 1.5 s);
- 时间戳显示零重叠 —— 每次都是
ws-b在ws-a结束后约 20 ms 才开始。workspace 一直是串行的,带不带 flag 都一样; - 该 flag 唯一可观测的副作用是在子进程环境里设置
npm_config_parallel=true。没有任何东西读它:本仓库零命中,整个已安装依赖树也零命中(我用正对照npm_config_user_agent确认过扫描本身是有效的)。
3. -- 之后的参数透传没有受影响
这是唯一一处不那么显然的风险:在 test:release 里该 flag 正好紧挨着 -- 分隔符。删除后透传依然正确 —— 真实仓库在 npm 12 下,合并后的树会打印 npm notice run vitest run --coverage.enabled=false;fixture 里带与不带该 flag,每个 workspace 收到的都是完全相同的 args=["--coverage.enabled=false"]。
4. 向后兼容,也解释了 CI 为什么没发现这个问题
npm 10.9.7 与 11.19.1 跑合并后的树都正常。CI 不可能发现原来的问题:release/CD workflow 固定用 npm@11.19.0,其余地方用 Node 22.x 自带的 npm(10.x)—— 都是接受该 flag 的版本。所以这在今天是开发机上的故障,等那些 pin 升到 12 的那天就会变成 CI 故障。
5. 两处测试改动到底有没有约束力?
我把每个守卫都变异回它本应拦住的那个回归,跑一遍套件,再恢复现场:
| 探针 | 变异 | 结果 |
|---|---|---|
| M1 | 给 test:release 加回 --parallel |
✅ 拦住 —— 全等值断言有效 |
| M2 | 给 test 和 test:ci 加回 --parallel |
❌ 拦不住 —— 见下 |
| M3 | 在 windows-runner-smoke.yml 里把 npm run test:ci 就地展开 |
✅ 本 PR 拦住,❌ main 拦不住 |
M3 值得单独说:把禁止子串从 npm run test:ci --workspaces --if-present --parallel 缩短为 npm run test:ci --workspaces --if-present,使这条断言严格变强了,因为更短的串能匹配到更多情况。它现在能拦住的那类 smoke workflow 回归,正是今天 main 拦不住的。这处改动是一个小改进,不只是机械的连带修改。
6. 对今天 main 的可合并性
这个 PR 是 8 月 8 日拉出去的,它改动的两个测试文件此后在 main 上变动很大,所以我没有只信 mergeable 标记,而是实际合并后做了测试:
git merge干净 —— 3 个文件,+5 −5,无冲突;- 合并后的树上,被改的两个测试文件全绿:31/31;
- 残留扫描在
--parallel上只剩packages/core/src/permissions/shell-semantics.ts里 coreutilssort的参数表,与本 PR 无关; - 合并后的树完整跑
npm run test:scripts:4 failed | 1934 passed | 30 skipped。这 4 个都不是本 PR 造成的。其中 3 个在另一个不带此 PR 的 main checkout 上同样复现(它们断言目录不可写,而 root 用户造不出这种目录);第 4 个install-script.test.js在已构建的 checkout 上是通过的,这里失败只是因为我的验证 worktree 从未构建过,缺少packages/audio-capture/dist。
关于是否给 test 和 test:ci 加断言这条讨论
先给一个实测数据,而不是先给观点:把 --parallel 加回 test 和 test:ci 后,今天 main 上的完整 scripts 套件结果逐字节一致 —— 4 failed | 1934 passed | 30 skipped,共 1968 个测试,没有一处变红。只有 test:release 被钉住了,所以这个 PR 修的那个回归可以悄无声息地回到两个最常用的脚本上。
话虽如此,我认同 @imrehg 的判断,也不认为这该拦住本 PR。字符串断言只钉住一个 flag 名字,而且在 npm 12 发布之前它根本抓不到这个 flag;真正能覆盖这一类问题的,是在更新的 npm 上跑套件。如果之后有人想要双保险,对三个脚本做一次 not.toContain('--parallel') 循环也就两行 —— 但那是一个独立且可选的改动,我更希望看到 npm/node 版本矩阵那个思路被推进。
我这次验证的边界
我验证的是三个脚本在 npm 12 下都能通过参数解析并进入 vitest,而不是整个 workspace 套件在那上面全绿 —— 那是一次很长的运行,且与本改动无关,本改动只影响参数解析。除此之外,上面的每一条都是实际跑出来的,不是推断的。
🤖 Generated with Claude Code — Claude Opus 5 (1M context)
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the pre-execution risk screen refused this sponsored run: the diff adds or modifies an npm lifecycle script. A maintainer who has reviewed the diff can run the verification manually in a disposable environment. 中文 — 判定:
|
|
Thanks for the PR! Template looks good ✓ — every required section is present and filled in (tiny aside: the 中文说明 block is left empty; not blocking). Problem: observed, not theoretical. #8721 ships a full reproduction — Direction: aligned. This is a developer-machine break today and a CI break the day the npm pins move to 12 — CI currently runs npm 10.x/11.x, which still tolerate the flag, so nothing in-repo catches it. Keeping Size: not applicable — no core paths. 3 files, +5/−5: three root Approach: this is the minimal fix — delete the never-supported flag and update the assertions that pin the exact script strings; there is nothing left to cut. One detail worth naming: the forbidden-substring guard in the no-AK integration CI test becomes strictly stronger after the edit, since the shorter string matches more. The only non-obvious risk was Risk: no high-risk paths touched, no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓——所有必填小节都在且已填写(小提示:中文说明部分是空的,不阻塞)。 问题:已观测到的 bug,不是理论问题。#8721 提供了完整复现——在 npm 12.0.2 下 方向:对齐。这在今天是开发者本机的问题,等 npm 依赖升到 12 就会变成 CI 问题——目前 CI 用的 npm 10.x/11.x 还能容忍这个 flag,所以仓库里没有任何机制能发现它。保证 规模:不适用——未触及核心路径。3 个文件,+5/−5:根 方案:这就是最小修复——删掉从未受支持的 flag,并更新固定脚本字符串的断言;没有可再砍的内容。值得一提的细节:no-AK 集成 CI 测试里的"禁止子串"断言在修改后变得更严格,因为更短的子串能匹配到更多内容。唯一不那么明显的风险在 风险:未触及高风险路径,无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff, my independent plan from the title + motivation was: remove Checking the base tree confirms the sweep is complete: Both test edits are mandatory, not drive-bys: No blockers, no convention issues. Test evidence
All 76 check-runs on the reviewed commit completed with zero failures. The macOS/Windows test lanes are skipped by design, not by this PR: Two honest caveats about what that green proves. First, CI runners bundle npm 10.x (Node 22) and the release pins sit at npm 11 — versions that accepted the flag — so the suite passes with or without this diff; CI cannot pin the change. Second, the sponsored What does settle the behavioral claim is the maintainer verification upthread (wenshao's own, not the author's claim): real npm 10.9.7 / 11.19.1 / 12.0.2 binaries shimmed onto 中文说明代码审查:在读 diff 之前,我根据标题和动机给出的独立方案是:从所有带 在 base 分支上核实: 两处测试改动都是必须的,不是顺手改: 无阻塞问题,无规范问题。 测试证据:被审提交上全部 76 个 check 已完成,零失败(详见上表)。macOS/Windows 测试通道为设计内跳过,与本 PR 无关: 两点如实说明:其一,CI 运行环境是 npm 10.x(Node 22)/npm 11,都能容忍该 flag,所以这套测试在本 diff 前后都会通过,CI 本身无法钉住这个改动;其二,受赞助的 真正坐实行为结论的是上文的维护者验证(wenshao 本人操作,而非作者自述):用真实的 npm 10.9.7 / 11.19.1 / 12.0.2 三个二进制并通过 shim 让脚本内嵌套调用的 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — a real observed break, the minimal correct fix, green CI, and a maintainer has already proven it end-to-end against real npm 10/11/12, including a merge into today's main. Stepping back: my independent proposal from the review stage was identical to what this PR does — remove the never-supported flag, update the two guards that pin the script strings, touch nothing else. That is exactly the shape of the diff, +5/−5 across 3 files, with no drive-by changes. The problem is unambiguous (npm 12 turned unknown-flag warnings into errors; The CI suite cannot pin this change (runners ship npm < 12) and the sponsored CI is fully settled on the reviewed commit — no pending runs. LGTM, approving. ✅ 中文说明置信度:5/5 —— 真实观测到的问题、最小且正确的修复、CI 全绿,且维护者已在真实 npm 10/11/12 环境端到端验证(含合并进当天 main)。 整体回顾:我在审查阶段的独立方案与本 PR 完全一致——删掉从未受支持的 flag、更新固定脚本字符串的两个断言、不动其他任何地方。diff 正是这个形状:3 个文件 +5/−5,无顺手改动。问题明确(npm 12 将未知 flag 警告升级为错误; CI 无法钉住这个改动(运行环境 npm < 12),受赞助的 被审提交上的 CI 已全部结束,无 pending 任务。LGTM,予以批准。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.23.0. |




What this PR does
The
--parallelflag was a proposal fornpm, but it wasn't merged in the end. With version 12 unknown flags are now errors instead of warnings (release notes, near the bottom of the Breaking Changes), so on that version it blocks runningnpm run testand similar tasks.Why it's needed
Without this change, on newer npm versions (>=12) the test trigger won't work at all.
Reviewer Test Plan
How to verify
Update to npm version >=12.0.0,
Run
make testfor example, and observe it workingEvidence (Before & After)
Before:
After: the test runs just fine
Tested on
Environment (optional)
npm run test, etc..Risk & Scope
Linked Issues
Closes #8721
中文说明