Repository navigation
fix(web-shell): isolate npm pack diagnostics from verifier output - #12367
Conversation
|
Edited 2026-09-20T23:3xZ, ~30 min after posting, correcting two identifiers in my own measurements and nothing else: the two 32-hex tokens I had printed beside the head/base verifier were md5sums of my scratch copies, and are now the git blob shas anyone can resolve ( Independent verification report — executed A/B at Verdict: mergeable. No blocking (Critical) defect found. This comment carries no approval; it records measurements only, as of the state read immediately before posting. It says nothing about any later commit. Why this is not a tmux reportThe verification task asks for a tmux TUI drive. That arm cannot reach this change, and I tested the precondition rather than assuming it: Arm 1 —
|
| arm | stdio |
bytes reaching parent stderr | returned stdout |
|---|---|---|---|
| base | (absent) | 15 (WARN_ON_STDERR) |
intact |
| head | ['ignore','pipe','pipe'] |
0 | intact |
So the first reading is the one that applies: capture was already happening, and the change removes only the echo. That is precisely the defect being fixed — npm's diagnostics were leaking into the verifier's own stderr.
The failure path loses nothing. With a child that writes to stderr and exits non-zero, at both arms error.message is byte-identical ("Command failed: …\nnpm_error_detail\n") and error.stderr is "npm_error_detail". The only difference is the parent-stderr echo (17 bytes → 0). Since the catch reports error.message into problems, and problems.length > 0 ends in process.exit(1), npm's diagnostics still reach the Refusing to publish output — and the guard fails closed, so swallowing the echo cannot let a broken package through.
Real-world trigger confirmed on npm 11.16.0: NPM_CONFIG_USERCONFIG pointing at an npmrc holding ELECTRON_MIRROR=… makes npm pack --dry-run --json --ignore-scripts emit a 156-byte npm warn Unknown user config "ELECTRON_MIRROR" on stderr while still exiting 0 with valid JSON on stdout.
Arm 2 — the A/B question the automated review named as load-bearing
It asked specifically "whether the new keeps npm warnings out of the verifier output case actually fails when the stdio line is reverted", and noted a green suite cannot answer that. I lifted the PR's own test (it imports only node:* and vitest — @qwen-code/* import count 0, so it needs no build and no stub) into a scratch tree beside each arm's verifier, all four blobs byte-faithful (size == len(b64decode(content)) and sha1('blob '+len+'\0'+bytes) == api.sha on every one):
| cell | test | verifier | stdio: line |
vitest reporter line |
|---|---|---|---|---|
| A | head | head (blob 42648faf45dc, 3678 B) |
present | Test Files 1 passed (1) / Tests 5 passed (5) |
| B (cross) | head | base (blob 1db0fc28194f, 3574 B) |
absent | Test Files 1 failed (1) / **`Tests 1 failed |
Cell B fails on exactly the new case: × keeps npm warnings out of the verifier output → AssertionError: expected 'npm warn Unknown user config "ELECTRO…' to be ''. The four pre-existing cases pass in both cells, so the red is bound to the diff and not to the harness. The new regression test does bind to the stdio line. A direct fixture-level reproduction agrees arm-for-arm (stderr="" at head, the 156-byte warning at base).
Arm 3 — one mechanism that makes that test pass vacuously today, not in a future npm
The automated review's finding 1 is that nothing asserts npm actually warned, so the case is green with or without the fix on an npm that stays quiet, and it noted the warning's own text says it "will stop working in the next major version of npm". I hit a present-tense instance of the same shape, and it is worth recording because it also explains why CI green cannot settle the question.
Launching the suite through npx put npm_config_userconfig into the runner's environment (alongside npm_lifecycle_event, npm_command, npm_execpath, …) — npm injects that whole family into any child it spawns. The test builds its child env as { ...process.env, NPM_CONFIG_USERCONFIG: <fixture npmrc> }, i.e. uppercase, while the inherited one is lowercase; npm resolves config-from-env case-insensitively, so the inherited variable shadowed the injected one, npm read the real userconfig, emitted no warning, and cell B passed 5/5 with the base verifier in place — a false green, confirmed by md5 on the file the suite was reading (9df4eaea…, stdio: hits 0). Printing Object.keys(process.env).filter(k => /npm/i.test(k)) beside the assertion is what localised it. Invoking the runner's binary directly with node, so npm injects nothing, gives the discriminating result in the table above (npm_config_userconfig ABSENT in both cells).
This matters for the lane, not just for my scratch: the file is not in HELPER_TESTS (that list is .github/scripts/** plus one integration-tests/** file, run by node --test); it is reached through test:scripts = vitest run --config ./scripts/tests/vitest.config.ts, which test:ci invokes as an npm run-script — so the runner environment there carries the same npm_config_* family. Non-blocking, and I am not filing it as a finding: the product change is correct and fails closed regardless, and the automated review already occupies this axis with the right remedy (a positive control that runs npm against the same throwaway userconfig and asserts the warning appears, or it.skipIf when it does not, so the guard degrades loudly). My measurement only supplies the reason to do it now rather than at the next npm major.
CI at this head, measured rather than read from the rollup
statusCheckRollup reads PENDING because one bot suite is still queued; that is not the product state. check-runs total_count=46 fetched=46 MATCH=True, 37 distinct lane names, NON-GREEN latest-per-name = [], and check-suites 9/9 MATCH=True CENSUS_VOID=False action_required=0 — so this is not the fork first-time-contributor gate and the tests were really permitted to run. Six product lanes executed and succeeded: Test (ubuntu-latest, Node 22.x) 21:32:03→22:18:43Z, Lint & Static →21:45:43Z, web-shell E2E Smoke 22:18:46→22:33:57Z, Integration Tests (no-AK, No Sandbox) →21:43:21Z, Desktop Shell (ubuntu-22.04) and (windows-2022). Test (macos), Test (windows) and Integration Tests (CLI) were skipped. The honest sentence is "all executed product lanes are green at this head", never "CI is green".
Cover
changed_files=2, +32/−3. Buckets {PRODUCTION:1, CI-INFRA:1}, sum 2 == changed_files. The single production file, packages/web-shell/scripts/verify-publish-artifacts.mjs (97 lines), was read in full, and the local diff of the two byte-faithful blobs reproduces the API's own +3/−1 in 2 hunks, so no line of the production delta is unseen. FILE-COMPLETE 1/1.
Coordination note, not a defect
This PR shares its base eceaede18ea5 with #12364, and both modify this same verifier — #12364 adds the wildcard-exports branch — measured at its live head 475e25981149, the file is 121 lines (blob a2fe45ac09f2) against 95 at the shared base eceaede18ea5 (blob 1db0fc28194f), and it carries Object.entries(pkg.exports) with the two-sided key.includes('*') && entry.includes('*') gate — while this one adds the stdio option (around :12–:27) and its own head is 97 lines (blob 42648faf45dc) still holding the base's Object.values(pkg.exports) form with no wildcard branch at all. The regions are disjoint, so a rebase should be clean, but whichever merges second will need one. Worth noting only so that a reader of this head does not mistake the absent wildcard branch for a revert of #12364's fix: it was never in this PR's base.
Not verified, with reasons
macOS and Windows behaviour (those lanes are skipped here; the author tested Windows). The self-hosted install branch, which installs chromium webkit-style browser sets without --with-deps elsewhere in the repo — unreachable for this file, which no workflow runs directly. I did not re-run the full test:scripts suite, only the one file under test.
独立验证报告 —— 在 a167dd82c7b8 上执行的 A/B
结论:可以合入。未发现阻塞性(Critical)缺陷。 本评论不包含 approve,只记录实测数据,且仅对发帖前一刻读到的状态负责,对之后的任何提交不作断言。
- 为什么不是 tmux 报告:已实测前置条件而非假设 —— 全仓库对
verify-publish-artifacts.mjs的引用只有packages/web-shell/package.json:47的prepublishOnly一处,packages/cli/src与packages/core/src中为 0 处,因此不存在交互式入口,tmux 驱动会在没有执行任何改动行的情况下变绿。 - 臂 1:用 node v24.18.1 实测
execSync语义,坐实了自动评审未能执行探针的那一点 —— base 有 15 字节子进程 stderr 回显到父进程,head 为 0;失败路径上两臂的error.message与error.stderr逐字节相同,诊断信息没有丢失,且problems.length > 0 ⇒ process.exit(1),守卫失败即拒绝(fail-closed)。npm 11.16.0 下NPM_CONFIG_USERCONFIG确实产生 156 字节 warning 且退出码为 0。 - 臂 2:把 PR 自带的测试抬升运行(仅依赖
node:*与vitest,@qwen-code/*引用数为 0),A 格 head+head =Tests 5 passed (5),B 格 head 测试 + base 校验器 =Tests 1 failed | 4 passed (5),失败信息为AssertionError: expected 'npm warn Unknown user config "ELECTRO…' to be ''—— 正是自动评审点名的承重问题:回退stdio那一行后新用例确实会失败。四个 blob 全部通过SIZE_MATCH与BLOBSHA_MATCH校验。 - 臂 3(非阻塞):发现一个当下就会让该用例空转通过的机制 —— 通过
npx(或任何 npm run-script,CI 的test:scripts正是如此)启动时,环境里已带有小写的npm_config_userconfig,而测试注入的是大写NPM_CONFIG_USERCONFIG;npm 读取配置环境变量时不区分大小写,于是注入值被遮蔽,B 格在装着 base 校验器(md59df4eaea…)时5/5 全绿。自动评审已就同一轴给出了正确的加固方案(正向对照或it.skipIf),我不重复提为问题,只补充"现在就做"的理由。 - CI:
rollup=PENDING来自一个排队中的 bot suite;产品通道实测 6 条执行并成功(含Test (ubuntu)、Lint & Static、web-shell E2E Smoke),NON-GREEN=[],action_required=0,因此不是 fork 首贡献者闸门。准确表述是"该 head 上所有已执行的产品通道为绿",而不是"CI 全绿"。 - 覆盖:唯一的生产文件(97 行)已完整读完,本地对两份逐字节忠实的 blob 做 diff 得到
+3/−1、2 个 hunk,与 API 自身数字一致 ⇒FILE-COMPLETE 1/1。 - 协作提示(非缺陷):本 PR 与 fix(web-shell): verify wildcard export targets against the packed file list #12364 同基
eceaede18ea5且改同一文件,区域不重叠,后合入者需 rebase。实测:共享基线该文件 95 行(blob1db0fc28194f),fix(web-shell): verify wildcard export targets against the packed file list #12364 现 head475e25981149为 121 行(bloba2fe45ac09f2,已带Object.entries与两侧通配判断),本 PR heada167dd82c7b8为 97 行(blob42648faf45dc,仍是基线的Object.values形式、无通配分支)。请勿把本 head 上没有通配exports分支误读为回退了 fix(web-shell): verify wildcard export targets against the packed file list #12364 的修复 —— 它本来就不在本 PR 的基线里。
qqqys
left a comment
There was a problem hiding this comment.
Approve — cashing in our own mergeable verification report, at the same head
Approved at a167dd82c7b8, which is still the live head and is the exact commit our earlier report reviewed.
Our verification report on this PR (5753300370, posted 2026-09-20T23:01:03Z, edited 23:36:30Z) concluded "Verdict: mergeable. No blocking (Critical) defect found." and stated that it carried no approval. A qwen-code-ci-bot approval is live at this same head (5261961583, 2026-09-20T22:34:17Z). That satisfies both conditions our review task sets for approving, so this row discharges the half the report deliberately left open.
What I re-measured immediately before posting, rather than carrying forward
- Head unmoved.
a167dd82c7b813b3a415d6b8d3fbcd241fc7b29a,commits=1,changed_files=2+32/−3, andmerge_base == base.sha == eceaede18ea5, so two-dot and three-dot diffs agree and no base-arm ambiguity applies. - No blocking finding from any producer — a measured zero, not an unmeasured one.
pulls/12367/commentsreturns 0 rows, socritAtHead(original_commit)=0andcritEver=0. The sole review row is the ci-bot approval. All fourqwen-code-ci-botissue comments (335 / 5,046 / 11,899 / 4,929 chars) were read in full:[Critical]hits 0,qwen-review-ledgerrows 0,"sev":"C"hits 0. There is no bot finding ledger on this PR at all, which is why the open-Critical count is stated as measured rather than as "no ledger row exists". - CI, stated precisely.
check-runs46/46MATCH=True;check-suites9/9MATCH=Truewithaction_required=0, so this is not GitHub's fork first-time-contributor gate and the tests were genuinely permitted to run. 6 product lanes executed and all succeeded at this head, each completing after the head's owncommittedDate(21:28:59Z):Test (ubuntu-latest, Node 22.x)21:32:03→22:18:43Z,Lint & Static→21:45:43Z,web-shell E2E Smoke22:18:46→22:33:57Z,Integration Tests (no-AK, No Sandbox)→21:43:21Z,Desktop Shell (ubuntu-22.04)andDesktop Shell (windows-2022). Three product lanes areskipped(Test (macos),Test (windows),Integration Tests (CLI)). The accurate sentence is therefore "every executed product lane is green at this head", not "CI is green":statusCheckRollupstill readsPENDINGbecause one bot lane (review-pr) is queued, and a rollup value is not a CI state. - No duplicate. This PR had zero
qqqysreview rows before this one. That was checked onpulls/12367/reviewsimmediately before the attempt and again immediately after it landed.
Scope of this approval, so nobody over-reads it. The one production file is packages/web-shell/scripts/verify-publish-artifacts.mjs. In .github/CODEOWNERS at this PR's head, @qqqys appears exactly once, on the /packages/core/ line, and no line covers packages/web-shell/ or scripts/; main's ruleset requires {required_approving_review_count: 1, require_code_owner_review: true}. So this row is additive — it does not by itself satisfy the code-owner requirement and is not expected to unblock the merge (ms=BLOCKED, rd=REVIEW_REQUIRED at post time). A maintainer's approval is still what merges this.
Everything above is as of the state read immediately before posting, at commit a167dd82c7b8. It says nothing about any later commit.
批准 — 兑现我们自己"可合入"的验证报告(同一 head)
在 a167dd82c7b8 批准:这仍是当前 head,也正是我们此前验证报告所评审的那个 commit。该报告结论为 "Verdict: mergeable. No blocking (Critical) defect found.",并明确声明不携带批准;而 ci-bot 已在同一 head 上给出批准(5261961583,22:34:17Z),任务规定的两个前置条件均已满足。
发布前重新实测(未沿用旧值):head 未移动(merge_base == base.sha == eceaede18ea5);pulls/12367/comments 为 0 行,四条 ci-bot 评论全文读取后 [Critical]、qwen-review-ledger、"sev":"C" 命中均为 0,故"无阻塞缺陷"是实测的零而非未测;check-suites 9/9 且 action_required=0(不是 fork 首贡献者闸门),6 条产品通道在该 head 上执行并全部成功,且完成时间均晚于 head 的 committedDate,另有 3 条产品通道 skipped —— 准确表述是"该 head 上所有已执行的产品通道为绿",而非"CI 全绿"(rollup=PENDING 来自一条排队中的 bot 通道 review-pr);本 PR 在此之前 qqqys 评审行为 0,发布前后各校验一次,不存在重复批准。
批准范围:唯一生产文件为 packages/web-shell/scripts/verify-publish-artifacts.mjs,该 PR head 上的 .github/CODEOWNERS 中 @qqqys 仅出现在 /packages/core/ 一行,未覆盖 packages/web-shell/ 或 scripts/,而 main 的规则集要求 require_code_owner_review: true。因此本条批准是附加性的,本身不满足 code owner 要求,也不预期解除合并阻塞(发布时 ms=BLOCKED、rd=REVIEW_REQUIRED);合入仍需维护者批准。
以上均为发布前即时读取的状态,对应 commit a167dd82c7b8,对之后的任何提交不作断言。
|
Correction to one inference in my approval A submitted review body cannot be edited through the API ( That approval's scope paragraph said this row is purely additive, that it does not by itself satisfy the review requirement because no Measured after posting, this PR is So Unchanged and still as measured immediately before posting: head |
|
This change correctly isolates npm pack diagnostics from the verifier output and addresses #12351. |
What this PR does
Keep
npm pack --dry-run --json --ignore-scriptsdiagnostics inside the publish verifier instead of inheriting them into the parent test process, and correct the comment that describes which lifecycle hook the flag protects against.Why it's needed
Issue #12351 reproduces a false failure in the verifier's happy-path silence assertion when npm emits a warning for an unrecognised user configuration key. The pack command still succeeds, but its inherited stderr reaches the test's
result.stderr, so a developer or CI runner with that configuration failsexpect(result.stderr).toBe('')for a reason unrelated to the package being verified.npm packdoes not runprepublishOnly; it can run pack-time hooks such asprepackandprepare. The existing--ignore-scriptsflag remains useful as defensive protection against those hooks re-entering the verifier, but its comment named the wrong lifecycle hook.Reviewer Test Plan
How to verify
Run
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/web-shell-publish-artifacts.test.js.The new regression creates a throwaway npmrc with an unknown
ELECTRON_MIRRORkey and passes it throughNPM_CONFIG_USERCONFIG. Before this change, the real verifier exits successfully but the test captures npm's warning on stderr and fails the exact-silence assertion. With this change, npm's stderr is captured byexecSync, the verifier remains silent on its happy path, and real pack failures still remain available through the thrown error message.The suite also keeps the four existing artifact cases green: missing pack members fail closed, missing build output fails closed, and the actual published entry shape remains accepted. The local hook probe confirmed that
npm pack --dry-runrunsprepack/preparebut notprepublishOnly, while--ignore-scriptssuppresses the pack-time hooks.Evidence (Before & After)
N/A — this is a release-verifier and test-harness change with no user-visible UI.
Tested on
Environment (optional)
Windows, Node.js v24.15.0, npm 11.12.1, Vitest 3.2.7.
Risk & Scope
error.messageanderror.stderrretain child stderr whenstdiousespipe.packages/browser-useand shared SDK type artifacts; the target JavaScript suite, ESLint, Prettier, syntax checks, and diff check pass. Wildcard export handling in web-shell: publish verifier rejects wildcard export targets #12332 / PR fix(web-shell): verify wildcard export targets against the packed file list #12364 is intentionally untouched.Linked Issues
Fixes #12351
中文说明
这个 PR 做了什么
让
npm pack --dry-run --json --ignore-scripts的诊断信息留在发布校验脚本内部,而不是继承到父级测试进程;同时修正注释,准确说明该 flag 防止的是哪类生命周期钩子重入。为什么需要
Issue #12351 复现了发布校验脚本 happy path 的静默断言会被 npm 用户配置警告误伤:当 npm 对未知用户配置项发出 warning 时,pack 命令本身仍然成功,但继承的 stderr 会流入测试的
result.stderr,于是开发者或 CI runner 会因与被测 package 无关的环境配置而使expect(result.stderr).toBe('')失败。npm pack不会运行prepublishOnly,但可能运行prepack、prepare等打包期钩子。现有--ignore-scriptsflag 作为防御措施仍然有价值,可以避免未来这些钩子重入校验脚本;只是原注释写错了生命周期钩子。评审验证方式
运行
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/web-shell-publish-artifacts.test.js。新增回归测试创建一个含未知
ELECTRON_MIRROR配置项的一次性 npmrc,并通过NPM_CONFIG_USERCONFIG传入。修改前,真实校验脚本虽然成功退出,但 npm warning 会被测试捕获到 stderr,导致精确静默断言失败。修改后,npm stderr 被execSync捕获,校验脚本的成功路径保持静默,而真实 pack 失败仍可从抛出的错误消息中获得。现有四个产物用例也保持通过:缺失的打包成员继续 fail-closed,未构建的输出继续 fail-closed,真实发布入口形状仍被接受。本地 hook probe 确认
npm pack --dry-run会运行prepack/prepare而不会运行prepublishOnly,--ignore-scripts会抑制打包期钩子。证据(修改前后)
不适用——这是发布校验器与测试 harness 的改动,没有用户可见 UI。
测试环境
环境(可选)
Windows、Node.js v24.15.0、npm 11.12.1、Vitest 3.2.7。
风险与范围
error.message与error.stderr均保留子进程 stderr。packages/browser-use的无关基线类型错误、共享 SDK 类型产物不一致阻塞;目标 JavaScript 套件、ESLint、Prettier、语法检查和 diff-check 已通过。web-shell: publish verifier rejects wildcard export targets #12332 / PR fix(web-shell): verify wildcard export targets against the packed file list #12364 的 wildcard export 处理刻意不触碰。关联 Issue
Fixes #12351