Repository navigation
fix(cli): preserve runtime boundaries during standalone updates - #14934
Conversation
Preserve Node/Bun archive flavor and validate actual CLI loading, package identity, version and prepared installation identity before activation. Keep Node npm transport and restrict Bun registry lookup to trusted config. Integrates chiga0's update stack through ce67eda, with isolated fixtures and real Node/Bun runtime regression coverage. Co-authored-by: gary.gq <gary.gq@alibaba-inc.com>
R1 validation reportValidated source:
Local environment: macOS 26.6.2 arm64, pnpm 11.24.0. The three focused files were The runtime entry was
The scenarios cover runtime flavor, real interpreter identity, requested CLI version, prepared-installation identity changes, hostile preload/version/relaunch overrides, malformed manifests, activation and rollback snapshots. Before the fix, an independent production-wrapper counterexample showed that metadata-only Fixtures used owned temporary HOME/QWEN_HOME/TMP directories and loopback HTTPS. For reproducibility the local launch wrapper removed ambient loader/CLI overrides and proxy variables before starting the harness; deliberately hostile environment scenarios are then planted by the runner itself. No real installation was selected. Limits: this is a local POSIX synthetic-fixture result. It does not establish native Windows or Linux results, all five release targets, corporate proxy/CA compatibility, published signatures, or launching a complete published release after rollback. Node's query executor is injected, so that fixture establishes delegation rather than a real npm subprocess. The Node host observed 119 probes; Bun's builtin binding observer is unavailable, so no Bun probe-count/zero-allocation claim is made. The runner configures a trusted local CA but lacks an untrusted-CA rejection control; its The local evidence archive is 中文验证报告验证源码：
本地环境：macOS 26.6.2 arm64、pnpm 11.24.0。三个定向文件为 Runtime 入口为
场景覆盖 runtime 类型、真实解释器身份、请求 CLI 版本、预下载安装身份变化、恶意 preload/版本/重启覆盖、损坏 manifest、激活及回滚快照。修复前，独立生产启动器反例表明，主 CLI 缺失或损坏时，仅读元数据的 夹具使用自有临时 HOME/QWEN_HOME/TMP 目录及 loopback HTTPS。为保证可复现，本地启动 wrapper 在 harness 启动前移除继承的 loader/CLI 覆盖和代理变量，再由 runner 自己设置刻意构造的恶意环境场景。没有选择真实安装进行更新。 限制：这是本地 POSIX 合成夹具结果，不代表原生 Windows 或 Linux、全部五种发布 target、企业代理/CA、生产签名，或回滚后启动完整已发布版本的验收。Node 查询执行器是注入的，因此该夹具证明委托路径而非真实 npm 子进程。Node 宿主观测到 119 次探测；Bun 内建绑定 observer 不可用，因此不声称测得 Bun 探测次数或零分配。Runner 配置可信本地 CA，但没有不可信 CA 的拒绝对照； 本地证据目录为 CI follow-up — 2026-10-21The Ubuntu Node 22 unit job at the exact PR head The CLI summary is 43,575 passed, 1 failed and 104 skipped, plus that unhandled error. The CI command already used two retries; those failures remain part of the evidence. This PR stays Draft while we inspect the exact test and compare against the base. No product-regression or flaky-test conclusion has been made, and no blind rerun or weakened assertion is being used to claim success. 中文：精确 PR head 的 Ubuntu Node 22 全量单测失败，上面链接保留原始记录。本 PR 改动的三个 CLI 测试文件共 204 项全部通过；失败断言来自未改的 Hosted Harness 预派发取消测试，其等待状态尚未完成。同文件另报告一次未处理的 socket hang up；日志所指“最近测试”仅是定位线索。CLI 汇总为 43,575 通过、1 失败、104 跳过及 1 个未处理错误，CI 自带两次重试仍失败。PR 继续保持 Draft，正在核对精确源码和基线，不把它预判为产品回归或偶发失败，也不通过盲目重跑或放宽断言制造通过。 CI test repair — d41362fCurrent head: Before the repair, the original candidate and an independently installed/built base both passed the ordinary cases. Fixed 1500ms delays after the real journal append reproduced the one-second wait failure on both, and the late-ack variant also produced an unhandled rejection. This establishes reachable test-budget/cleanup failure mechanisms; it does not prove the cause of the original Ubuntu CI failure, which remains recorded above. Independent verification used Node 22.23.2 on macOS arm64 with retries disabled: full build and typecheck passed; the original pair passed (2 tests), and the two fixed-delay cases passed (2 tests). Two deliberately injected early/late assertion failures still exited 1 with only the intended assertion, no unhandled error, and cleanup completed in 19ms/3ms. They remain negative controls, not passing product tests. All five runs used identical source, artifacts and runtime, left no owned processes or scratch files, and required no forced termination. The complete Hosted Harness suite was not rerun locally. The committed test SHA256 is 中文：当前 head 为 修复前，候选与独立安装构建的基线在普通用例中均通过；在真实 journal append 后固定延迟 1500ms，两边均复现一秒等待失败，晚确认用例还产生未处理拒绝。这证明测试等待与清理机制存在可达问题，尚不能证明原 Ubuntu CI 的实际原因，原失败记录保留。 独立复验使用 macOS arm64、Node 22.23.2、零重试：完整 build/typecheck 通过，原始两例与两个延迟用例均通过。刻意注入的早/晚断言仍以 1 退出，仅报告目标断言，无未处理异常，清理分别约 19ms/3ms。这两项是负向对照，不算产品测试通过。五轮均使用相同源码、产物及 runtime，无自有进程或临时文件残留，也未强制终止；本地没有重跑整个 Hosted Harness 套件。提交后测试文件哈希与独立验收一致，正常提交检查通过。新提交 TUI/no-flicker 与无密钥集成检查已通过，其余 CI 仍在运行。独立复验后的修复现转为 Ready，使代码审查与剩余 CI 并行；合并验收仍需当前 head 的全部必要 CI 和审查结果。 |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
activeWorkStaleMs |
6 |
5 |
— Qwen Code · serve A/B
|
Thanks for the PR — this is a substantial one, so I read the surrounding release/packaging code before forming a view rather than taking the description at face value. Template looks good ✓ — every required section is filled in, the Tested-on table is honest about what wasn't covered, and the Chinese body matches the English one section for section. Problem: real, and observable from source rather than theoretical. On No closing references on this PR ( Direction: aligned. The Bun flavor is already in the tree — release scripts, preview installers, Size: Stage 0's core-module gate does not apply — nothing lands in Approach: I sketched my own plan before reading the diff (persist the flavor in the existing install manifest with a legacy Two scope questions, neither a blocker:
Risk: no elevated risk signals — none of the changed files match the revert-correlated paths from Stage 1e. The concern I do have isn't about the updater's logic; it's about where the new test harness is wired in. I've written that up in the code review rather than guessing at it here. Moving on to code review. 🔍 中文说明感谢贡献——这个 PR 分量不小，所以我先读了周边的发布/打包代码再下判断，而不是只看描述。 模板完整 ✓——所有必填小节都写了，Tested-on 表格如实标注了未覆盖的部分，中文正文与英文逐段对应。 问题： 真实存在，而且可以从源码直接观察到，不是理论性问题。在 本 PR 没有 closing 引用（ 方向： 对齐。Bun flavor 已经在仓库里——发布脚本、preview 安装脚本、 规模： Stage 0 的核心模块门禁不适用——没有改动落在 方案： 我在看 diff 之前先写了自己的方案（把 flavor 持久化到已有的安装 manifest 中，缺省按旧的 两点范围问题，都不是阻断项：
风险： 无升级风险信号——改动文件均未命中 Stage 1e 中与回滚相关的高风险路径。我确实有的顾虑不在更新器逻辑本身，而在新测试夹具被接入的位置。这一点我写在代码审查里，而不是在这里猜测。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI read the three production files closely and then went and checked every assumption they make about the rest of the repo, because most of the ways this could be wrong are ways it would be silently wrong — a path that doesn't match packaging, a gate that never fires. What I verified against the packaging pipeline (all consistent):
One reachability check worth putting on the record. Security handling looks careful and I couldn't find a way around it: registry protocol and embedded-credential validation, the auth walk-up that keeps a token from being sent to a sibling path, My actual concern is not the updater — it's where the harness got wired in.
So Related, and the part I'd most want changed: the pin's escape hatch doesn't permit a different version. The default entrypoint asserts Both of these are CI/release-reliability and maintainability calls, not correctness defects in the update path — I'm flagging them for a maintainer's decision rather than claiming the feature is broken. Minor, non-blocking:
sequenceDiagram
participant P1 as Update caller
participant P2 as standaloneUpdateTarget
participant P3 as Registry lookup
participant P4 as prepare and download
participant P5 as Manifest validation
participant P6 as Smoke test
participant P7 as Atomic replace
P1->>P2: read installed manifest
P2-->>P1: target, runtime (legacy default node), version, dir identity
P1->>P3: resolve dist-tags (bun native fetch, node via npm)
P3-->>P1: latest version
P1->>P4: fetch qwen-code-TARGET plus optional preview suffix
P4->>P5: extract and read downloaded manifest
P5-->>P4: name, target, runtime and version must match
P4->>P6: probe interpreter identity, CLI version, bun launcher help
P6-->>P4: pass, or reject before any replacement
P4->>P7: recheck installed identity then replace
P7-->>P1: done or deferred, rollback on failure
Files changed (11)
Testing evidenceThis is an unattended CI run, so per the gate's rules I did not build, run or execute anything from this PR. No tmux capture and no local test execution — the evidence below is the PR's own CI, read through the API for the reviewed commit, plus static verification of the packaging assumptions described above. There are no failures on this commit: 23 checks succeeded, 27 were skipped, 5 are still pending. The important pending one is Not verified, and why:
CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle this: 中文说明代码审查 我仔细读了三个生产文件，并逐一核对了它们对仓库其他部分的假设，因为这类改动出错的方式往往是"静默出错"——路径与打包不一致，或某个门禁根本不触发。 与打包流水线核对一致的部分：smoke test 的解释器路径与打包产物完全对应（POSIX 我最初担心新的下载 manifest 校验会打断现有 Node 更新，因为它强制要求 有一处可达性核对值得记录在案：整个 Bun 分支由 安全处理是细致的，我没有找到绕过方式：registry 协议与内嵌凭据校验、防止 token 发送到兄弟路径的逐级上溯、跨 origin 重定向时丢弃 我真正的顾虑不在更新器，而在夹具被接入的位置。 结果是 与此相关、也是我最希望修改的一点：版本固定的"逃生口"其实不允许换版本。 默认入口断言 这两点都属于 CI/发布可靠性与可维护性的取舍，不是更新路径的正确性缺陷——我提出来交由维护者决定，而不是断言功能有问题。 次要、非阻断：探测的 测试证据 这是无人值守的 CI 运行，按门禁规则我没有构建、运行或执行本 PR 的任何代码。 没有 tmux 抓屏，也没有本地测试执行——下面的证据是通过 API 读取的被审提交自身 CI 结果，加上对上文所述打包假设的静态核对。 该提交上没有失败：23 项成功、27 项跳过、5 项仍在进行。最关键的待定项是 未验证项及原因：夹具在 CI 中的实际行为（上述任务仍在进行，其在 ECS runner 上的真实耗时与 沙箱化验证可以定论： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the updater itself is sound and better-tested than the diff size suggests; what holds this back is the new test harness being wired into a publish-gating job with a version pin that can't be bumped without editing code, plus the one CI job that would prove the harness works hasn't reported yet. Going back to the proposal I wrote before opening the diff: this matches it and beats it in two places I hadn't thought of — rechecking directory identity ( I also went hunting for the failure mode that would matter most here — a new gate that silently rejects every legitimate update, or a new branch that never fires. Neither is present, and both were close calls worth naming:
On "would I curse the author in six months": mostly thank them, with one exception. The exception is the pin. The honest counterargument, which a maintainer may well accept: CI already depends on the network for installs, the harness checksum- and integrity-verifies everything it downloads, and a real-runtime matrix is exactly the kind of coverage this migration has been missing. If the answer is "yes, and we want it gating," that's a legitimate call — it just isn't mine to make, and it isn't in the PR's Risk & Scope section, so I don't think it's been made deliberately yet. Two smaller things I'd genuinely consider, neither blocking: moving the harness to its own non-gating job would keep the coverage while taking it off the publish path; and the What I'm deferring on, concretely. Three questions I can't answer from the diff, tests and description:
I am not requesting changes and not approving. The code review found no correctness, security or regression defect — the concerns above are infrastructure-tradeoff calls that belong to a maintainer. Note on escalation: I tried to resolve an accountable owner deterministically and could not — 中文说明Confidence: 3/5 — 更新器本身是可靠的，测试覆盖也比 diff 体量所暗示的更好；让我保留意见的是新测试夹具被接入了一个阻塞发布的任务，且其版本固定无法在不改代码的情况下调整；再加上唯一能证明该夹具可用的 CI 任务尚未给出结果。 回到我在看 diff 之前写的方案：本 PR 与之一致，并在两处我没想到的地方做得更好——预下载激活时重新校验目录身份（ 我也专门去找了这里最要紧的失败模式——某个新门禁静默拒绝所有合法更新，或某个新分支根本不触发。两者都不存在，而且都是差一点就成立的隐患，值得点名：
关于"六个月后我会不会骂作者"：大部分是感谢，只有一个例外。例外就是这个版本固定。 也需要给出诚实的反方观点，维护者完全可能接受：CI 本来就因安装而依赖网络，夹具对下载内容都做了 checksum 与 integrity 校验，而真实 runtime 矩阵正是这次迁移一直缺少的覆盖。如果结论是"是的，我们希望它阻塞发布"，那是个合理决定——只是不该由我来做；而且它没有出现在 PR 的 Risk & Scope 中，所以我认为这个决定目前还没有被有意识地做出。 两件更小的、我确实建议考虑但都不阻断的事：把夹具移到独立的不阻塞发布的任务，可以在保留覆盖的同时把它从发布路径上摘下来； 我具体在什么问题上转交。 三个我无法从 diff、测试和描述中回答的问题：
我没有请求修改，也没有批准。代码审查没有发现正确性、安全或回归缺陷——上述顾虑属于基础设施取舍，应由维护者决定。 关于转交： 我尝试以确定方式解析出负责人，但没能成功—— — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Follow-up on the three questions at exact head
The cancellation-test repair remains limited to the two CI-exposed cases with original assertions preserved and independent failure-cleanup controls recorded. I am retaining it in this cohesive delivery instead of opening another small prerequisite PR. No product changes, new version-override mechanism, or CI retry were made in this follow-up. 中文说明针对精确 head
取消测试修复仅覆盖 CI 暴露的两例，保留原断言和独立失败清理证据，继续放在本次完整交付中，避免再拆一个小型前置 PR。本次跟进没有修改产品、增加版本覆盖机制或重跑 CI。 |
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and their suites did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": none — I did not verify the untested premise in my finding's stderr-overflow trigger (that a global ~/.bunfig.toml preload can emit >64 KiB on stderr during t…; "agent reverse-audit (round 1)": none of my checks were cut short, but I did not read the serveArchive body past diff line 472 ( await tar.c(...) ) as *chunk* text — I read its post-change so…; "agent reverse-audit (round 1)": none — I did not verify whether the repo runs packages/cli unit tests on a Windows lane, which is why the win32-platform cells above were reasoned from the pr…; "agent reverse-audit (round 3)": none — I did not run the suite, so the ≈3.9 s per-case figure in my failure scenario is f13's measurement applied to this cell, not one I took myself.; "agent reverse-audit (round 1)": could not execute scripts/test-standalone-runtimes.mjs (or --ci ) to measure the worker's real wall clock — the run requires egress to nodejs.org and registr…, and 1 more.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and their suites did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：none — I did not verify the untested premise in my finding's stderr-overflow trigger (that a global ~/.bunfig.toml preload can emit >64 KiB on stderr during t…；"agent reverse-audit (round 1)"：none of my checks were cut short, but I did not read the serveArchive body past diff line 472 ( await tar.c(...) ) as *chunk* text — I read its post-change so…；"agent reverse-audit (round 1)"：none — I did not verify whether the repo runs packages/cli unit tests on a Windows lane, which is why the win32-platform cells above were reasoned from the pr…；"agent reverse-audit (round 3)"：none — I did not run the suite, so the ≈3.9 s per-case figure in my failure scenario is f13's measurement applied to this cell, not one I took myself.；"agent reverse-audit (round 1)"：could not execute scripts/test-standalone-runtimes.mjs (or --ci ) to measure the worker's real wall clock — the run requires egress to nodejs.org and registr…，另有 1 条。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| "test:scripts": "vitest run --config ./scripts/tests/vitest.config.ts", | ||
| "test:standalone:runtimes": "node scripts/test-standalone-runtimes.mjs", | ||
| "test:standalone:runtimes:ci": "node scripts/test-standalone-runtimes.mjs --ci", | ||
| "test:scripts": "npm run test:standalone:runtimes:ci && vitest run --config ./scripts/tests/vitest.config.ts", |
There was a problem hiding this comment.
[Suggestion] R1-1: Chaining the new runtime-provisioning step ahead of the scripts suite with && makes a network failure suppress the whole suite. test:standalone:runtimes:ci needs egress to nodejs.org and registry.npmjs.org (plus openssl), and undici's global dispatcher ignores HTTP_PROXY/HTTPS_PROXY, so on a restricted-egress or proxied runner — or during an upstream outage — the first half fails and vitest never starts. About 115 meta-test files then report nothing at all, so a genuine scripts regression in the same PR is invisible while the lane shows only a download error. Because test:ci and test:release both chain test:scripts, this reddens the PR gate (ci.yml:804) and the release publish gate (release.yml:460) for a reason unrelated to the change under test. npm run preflight also now pulls roughly 100 MB of runtimes.
Witness:
measured in this review: `node scripts/test-standalone-runtimes.mjs --ci`
produced zero output and had to be killed at 170 s (exit 124).
ci() asserts response.status === 200 against
https://nodejs.org/dist/v24.19.0/SHASUMS256.txt
https://registry.npmjs.org/@oven/bun-*/1.3.14
with AbortSignal.timeout(300_000) per fetch.
No workflow sets PINNED_NODE / PINNED_BUN / QWEN_TEST_NODE_BASE_URL
(grep over .github/: no matches).
Keeping the gate out of the default path avoids the coupling — leave test:scripts as vitest run --config ./scripts/tests/vitest.config.ts and invoke test:standalone:runtimes:ci from its own workflow step or job, where a network dependency is visible and retriable. Alternatively let ci() skip with an explicit one-line notice when the runtimes cannot be provisioned and no PINNED_NODE/PINNED_BUN is supplied, so both halves always execute.
ci.yml:804 invokes this as "${test_runner[@]}" npm run test:scripts -- "${retry_arg[@]}", and package-scripts.test.js:1177 asserts that step text, so npm appends --retry=N to the END of the compound string — any rewrite must keep vitest run --config ./scripts/tests/vitest.config.ts as the final command or the retry flag stops reaching vitest. package-scripts.test.js:1161-1163 also pins test:ci exactly, and :1208-1213 pins that exactly two commands touch test_runner. No existing test pins the content of test:scripts, so this fix needs its own assertion — one that a failing --ci leg does not suppress the vitest leg.
中文说明
用 && 把新的运行时下载步骤串在 scripts 测试套件前面，会让一次网络失败把整个套件吞掉。test:standalone:runtimes:ci 需要访问 nodejs.org 和 registry.npmjs.org（还需要 openssl），而 undici 的全局 dispatcher 不读 HTTP_PROXY/HTTPS_PROXY，因此在受限出口或走代理的 runner 上（或上游故障时），前半段失败后 vitest 根本不会启动。这样约 115 个 meta-test 文件会完全不产出结果，同一个 PR 里真正的 scripts 回归因此不可见，而流水线上只显示一个下载错误。由于 test:ci 和 test:release 都串了 test:scripts，这会让 PR 门禁（ci.yml:804）和发布门禁（release.yml:460）因为与被测改动无关的原因变红。npm run preflight 现在还会额外拉取约 100 MB 的运行时。
建议让这个门禁脱离默认路径：test:scripts 保持为 vitest run --config ./scripts/tests/vitest.config.ts，把 test:standalone:runtimes:ci 放到独立的 workflow step 或 job 中执行，这样网络依赖是可见且可重试的。或者让 ci() 在无法下载运行时且未提供 PINNED_NODE/PINNED_BUN 时打印一行明确提示后跳过，使两部分都能执行。
约束：ci.yml:804 以 npm run test:scripts -- "${retry_arg[@]}" 调用，npm 会把 --retry=N 追加到复合字符串的末尾，所以无论如何改写都必须让 vitest run --config ./scripts/tests/vitest.config.ts 保持在最后，否则 retry 参数不再传给 vitest；package-scripts.test.js:1161-1163 精确固定了 test:ci，:1208-1213 固定了只有两个命令接触 test_runner。目前没有测试固定 test:scripts 的内容，因此该修复需要自带断言。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| supertest(server).post(`/session/${SESSION_ID}/cancel`), | ||
| ) | ||
| .set('X-Qwen-Client-Id', created.body.clientId) | ||
| .timeout(SETTLE_TIMEOUT_MS) |
There was a problem hiding this comment.
[Suggestion] R1-2: This new request-level deadline is budgeted at one wait's length, but the request has to stay open across four waits plus a POST. .timeout(ms) in superagent arms a deadline at dispatch (inside Request.prototype.end), and supertest's Test extends Request, so the timer covers every wait the test runs before releaseCancelAck() — not just the settle afterwards. On a loaded runner where the first wait consumes most of SETTLE_TIMEOUT_MS, the deadline fires before the test ever releases the reply, aborting a request the test is deliberately holding open. The test then fails with Timeout of 45000ms exceeded although nothing is wrong with the pipeline — the same class of load-dependent flake this commit was pushed to repair.
Witness:
Both arms measured on this host (RUNNER_NAME=ecs-qwen-hk1-9, so
SETTLE_TIMEOUT_MS = 45000, testTimeout = 60000), with a modelled loaded
host placing delays inside every wait's own budget:
ARM A (PR code, .timeout present):
FAIL ... > targets the original active turn when the durable cancel
reply arrives late
Error: Timeout of 45000ms exceeded
Tests 1 failed | 603 skipped (604) <- 14 s of the 60 s ceiling unused
ARM B (identical host model, ONLY the .timeout line removed):
Tests 1 passed | 603 skipped (604)
Baseline (unloaded): passed in 466 ms.
Boundary: delay = SETTLE_TIMEOUT_MS - 700 => PASS;
delay = SETTLE_TIMEOUT_MS - 200 => one run aborted, one passed.
Arm B passes the same scenario, so dropping .timeout() and letting the new finally bound teardown is a verified fix rather than a hypothesis — that finally already releases finishModel(), releaseCancelAck(), finishNextModel?.() and drains cancelResponse with a .catch. If you prefer to keep an explicit deadline, size it from the sum of the waits it must span instead of from one wait.
This interacts with R1-3: giving the two pre-release waits SETTLE_TIMEOUT_MS budgets widens the legal worst case and makes this collision near-certain rather than narrow, so the two need sizing together. SETTLE_TIMEOUT_MS is defined at :375-380, and the adjacent comment asks that waits be budgeted through that slow-host policy rather than a fresh number.
中文说明
这个新增的请求级超时只按“一次 wait”的长度来预算，但该请求必须跨越四次 wait 加一次 POST 保持打开。superagent 的 .timeout(ms) 在派发时（Request.prototype.end 内部）就开始计时，而 supertest 的 Test extends Request，因此这个计时器覆盖的是 releaseCancelAck() 之前测试运行的所有 wait，而不只是之后的 settle。在负载较高的 runner 上，如果第一次 wait 就用掉了大部分 SETTLE_TIMEOUT_MS，超时会在测试释放应答之前触发，从而中止一个测试故意保持打开的请求。于是测试会以 Timeout of 45000ms exceeded 失败，但流水线本身没有任何问题——这正是本次提交想要修复的那类负载相关抖动。
实测两条对照臂（本机 RUNNER_NAME=ecs-qwen-hk1-9，故 SETTLE_TIMEOUT_MS = 45000、testTimeout = 60000，用注入延迟模拟高负载主机）：带 .timeout 的 PR 代码失败并报 Timeout of 45000ms exceeded（60 秒上限还剩 14 秒未用）；仅删掉这一行后同样场景通过。边界测量显示余量只有几百毫秒。
因此“删掉 .timeout()、让新增的 finally 负责收尾”是被实测验证过的修复，而不是猜测——该 finally 已经释放 finishModel()、releaseCancelAck()、finishNextModel?.() 并用 .catch 排空 cancelResponse。如果希望保留显式超时，应按它需要跨越的所有 wait 之和来设定。注意这与 R1-3 相互影响，需要一起定尺。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| payloadDigest: `sha256:${createHash('sha256').update(JSON.stringify(nextPrompt)).digest('hex')}`, | ||
| }) | ||
| .expect(202); | ||
| await vi.waitFor(() => expect(nextSignal).toBeDefined()); |
There was a problem hiding this comment.
[Suggestion] R1-3: The head commit's stated purpose is that waits use the existing host-aware budget, but three of the four vi.waitFor calls inside the hardened region still run on vitest's 1 s default — :17531, :17558 and :17564. Only the cancelCommitted wait at :17527 received { timeout: SETTLE_TIMEOUT_MS }.
:17558 is the exposed one: the POST /prompt has already returned 202, so this wait covers the asynchronous window until dispatch reaches state.model.mockImplementationOnce and assigns nextSignal. That is real work, not a post-gate settle. On a loaded runner it can exceed 1000 ms and the test fails although nothing is wrong with the pipeline — and this file's own rationale at :365-374 records that #14406 and #14023 both failed on exactly this, directing that waits be budgeted through the slow-host policy the file already applies at 255 other call sites.
Witness:
Enumerated against the real file (awk over :17440-17600):
:17527-17528 { timeout: SETTLE_TIMEOUT_MS } <- the only budgeted wait
:17531 no options
:17558 no options <- covers real async work
:17564 no options
Vitest's default read off the installed package rather than recalled:
node_modules/vitest/dist/chunks/vi.bdSIJ99Y.js
const { interval = 50, timeout = 1e3 } = ...
Adding { timeout: SETTLE_TIMEOUT_MS } to the three remaining waits — at minimum the nextSignal wait at :17558 — closes it. This is N/A for a fix witness: the change adds a timeout option inside a test file, and its value is the absence of a load-dependent flake rather than an assertable behaviour.
Size this jointly with R1-2: widening these waits also widens the legal worst case that R1-2's request deadline has to span.
中文说明
头提交的说明是“等待使用已有的主机感知预算”，但被加固区域内四次 vi.waitFor 中有三次仍然跑在 vitest 的 1 秒默认值上——:17531、:17558 和 :17564，只有 :17527 的 cancelCommitted 等待拿到了 { timeout: SETTLE_TIMEOUT_MS }。
其中 :17558 风险最大：POST /prompt 已经返回 202，这个等待覆盖的是派发流程到达 state.model.mockImplementationOnce 并赋值 nextSignal 之前的异步窗口，属于真实工作而不是门控之后的 settle。在负载较高的 runner 上它可能超过 1000 ms，于是测试失败但流水线并无问题。本文件 :365-374 的说明已记录 #14406 和 #14023 都因此失败，并要求通过文件已有的慢主机策略来预算等待（该策略在本文件另有 255 处调用）。
建议给剩下三次等待补上 { timeout: SETTLE_TIMEOUT_MS }，至少是 :17558 的 nextSignal 等待。此项无需 fix witness：改动只是在测试文件里加超时选项，其价值是消除负载相关抖动，而不是可断言的行为。请与 R1-2 一起定尺。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| fs.writeFileSync(path.join(fixture, 'qwen-code', 'lib', 'cli.js'), ''); | ||
| if (options.runtime === 'bun') { | ||
| fs.writeFileSync( | ||
| path.join(fixture, 'qwen-code', 'lib', 'cli-entry.js'), |
There was a problem hiding this comment.
[Suggestion] R1-4: The lib/cli-entry.js file this fixture now writes is never loaded by anything, so the Bun launcher guard it exists to exercise has no test that can fail on any host. The generated /bin/sh interpreter stub dispatches only on $1 = "-e"; production's launcher leg invokes [interpreter, [<install>/lib/cli-entry.js, '--help']], so $1 is a path and never -e, and both legs fall into the same unconditional else that prints 1.2.3 and exits 0. interpreterExitCode is emitted only inside the -e branch, so none of the seven knobs serveArchive gained can reach the launcher leg.
The consequence is that the exact regression this probe was added for — a Bun archive with a valid lib/cli.js but a corrupt lib/cli-entry.js, which bricks the install after atomicReplace — has no unit-level witness. Point standalone-update.ts:577 at the wrong path, drop the --help argv, invert if (launcher.exitCode !== 0), or delete the guard entirely, and the whole packages/cli suite stays green.
Witness:
Mutation (isolated copy): standalone-update.ts:581-583
if (launcher.exitCode !== 0) { throw new Error('Smoke test failed:
Bun launcher could not load CLI'); }
replaced by `void launcher;`
-> Test Files 1 passed (1), 136 tests, exit 0
Unmodified baseline: 136 passed, exit 0 (same number - the guard is
invisible to the suite)
Sweep (repo-wide, all *.ts/*.mjs/*.js): "could not load CLI" appears ONLY in
standalone-update.ts, its dist/ copy, and test-standalone-runtimes.mjs
-> 0 occurrences in any test file
Stub run as generated, against a cli-entry.js containing
'THIS FILE IS NEVER READ - CORRUPT @@@'
-> stdout "1.2.3", exit 0
Give the stub a launcher leg and a knob for it: add launcherExitCode?: number to the options type, and split the generated script's else so the launcher invocation is distinguishable from the version invocation — dispatching on "${1##*/}" = "cli-entry.js" while preserving printf "1.2.3\n" for cli.js. The new case should assert that a launcherExitCode: 1 archive rejects with Smoke test failed: Bun launcher could not load CLI and leaves manifest.json unchanged.
That case must go red when the if (launcher.exitCode !== 0) { throw … } block at standalone-update.ts:581-583 is deleted — a mutation that is green suite-wide today. Two constraints: the stub must keep answering the version leg with a bare semver, because production gates it at standalone-update.ts:549-566 with SEMVER_RE and semver.eq; and if the fix instead makes a real interpreter load the fixture's cli-entry.js, it must also add the module-type marker the runtime harness ships and this fixture omits — scripts/test-standalone-runtimes.mjs:706-709 writes lib/package.json as { type: 'module', version: '1.2.3' } next to the launcher, because this fixture writes cli-entry.js as import "./cli.js"; and Node rejects that without it.
中文说明
这个 fixture 新写入的 lib/cli-entry.js 实际上不会被任何东西加载，因此它本要验证的 Bun launcher 保护在任何主机上都没有可能失败的测试。生成的 /bin/sh 解释器 stub 只在 $1 = "-e" 时分派；而生产的 launcher 分支调用的是 [interpreter, [<install>/lib/cli-entry.js, '--help']]，$1 是路径而永远不是 -e，于是两条分支都落进同一个无条件 else，打印 1.2.3 并以 0 退出。interpreterExitCode 只在 -e 分支里输出，所以 serveArchive 新增的七个开关都无法作用到 launcher 分支。
结果是：这个探针正是为了防御的回归（lib/cli.js 正常但 lib/cli-entry.js 损坏的 Bun 归档，会在 atomicReplace 之后把安装弄坏）在单元层面没有任何见证。把 standalone-update.ts:577 指向错误路径、去掉 --help 参数、把 if (launcher.exitCode !== 0) 反写，或整块删掉该保护，整个 packages/cli 套件依然全绿。
建议给 stub 增加一条 launcher 分支和对应开关：在选项类型中加入 launcherExitCode?: number，并拆分生成脚本的 else，用 "${1##*/}" = "cli-entry.js" 分派，同时保留 cli.js 的 printf "1.2.3\n"。新用例应断言 launcherExitCode: 1 的归档会以 Smoke test failed: Bun launcher could not load CLI 拒绝，并保持 manifest.json 不变。删掉 standalone-update.ts:581-583 的保护块时该用例必须变红（今天这个变异是全绿的）。
约束：stub 仍须以裸 semver 应答 version 分支，因为生产在 standalone-update.ts:549-566 用 SEMVER_RE 和 semver.eq 做门控；如果改为让真实解释器加载 fixture 的 cli-entry.js，还必须补上运行时 harness 提供而本 fixture 缺少的模块类型标记——scripts/test-standalone-runtimes.mjs:706-709 会在 launcher 旁写入 { type: 'module', version: '1.2.3' } 的 lib/package.json，因为本 fixture 把 cli-entry.js 写成 import "./cli.js";，Node 缺少该标记会拒绝加载。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| 'bin', | ||
| runtime, | ||
| ); | ||
| fs.copyFileSync(options.interpreterPath, interpreter); |
There was a problem hiding this comment.
[Suggestion] R1-5: Every interpreterPath: process.execPath fixture copies, gzips, re-downloads and extracts the host's ~124 MB interpreter binary, and that packing cost is about 99% of this file's runtime. Five serveArchive call sites expand through their it.each into 17 such cases, each writing roughly 380 MB (copy + archive + readFileSync + Uint8Array copy + download + extraction).
The concrete cost is ~66 s and about 6.5 GB of transient I/O added to every local run and every CI shard of this one file, in a repo whose CI already documents disk-pressure and load spikes on these runner pools. The other 119 cases cost under half a second combined.
Witness:
Two consecutive full-file runs on the unmodified tree:
136 passed, tests 66.70 s / 66.82 s
Per-case distribution parsed from the junit report (external authority):
cases >= 1.0 s : 17, summing 66.37 s (99.3% of test time)
cases < 1.0 s : 119, summing 0.45 s
/usr/bin/node = 124,836,408 bytes
Isolated pack probe: 4.73 s -> 43,960,439-byte archive
Candidate fix APPLIED (`#!/bin/sh\nexec '<execPath>' "$@"` in place of
copyFileSync): 66.82 s -> 1.38 s (48x), 136/136 still green
NEGATIVE CONTROL (same wrapper replaced by a non-executing
`printf "1.2.3\n"` fake): 17 failed | 119 passed, and the 17 failures
are exactly the 17 slow cases by name
The negative control is what makes this safe to act on: it proves the exec wrapper preserves the real-runtime behaviour these cases depend on, rather than quietly vacuating it. Reserve the real-binary path for cases whose assertion genuinely requires a real JS runtime, and give the existing fake interpreter a version-output knob so the 'legal CLI version mismatch', 'preserves smoke outcome' and 'disguised as the other runtime' cases can drop interpreterPath entirely.
The constraint that decides which cases may move: the five preload/redaction cases depend on a real runtime honouring the injected options — vi.stubEnv('NODE_OPTIONS', --require ${preload}) and vi.stubEnv('BUN_OPTIONS', --preload ${preload}) at :729-730, with the cliSource guard if (process.env.QWEN_UPDATE_TEST_ENV !== "preserved" || ...) process.exit(9) at :747-748. Routing those to a printf-only interpreter would make their pollution and redaction assertions vacuous rather than fast; they must keep spawning a real interpreter, which the exec wrapper satisfies. Verified on Linux with a Node host only — all 17 cases are skipIf(win32) and a Bun host was not exercised.
中文说明
每个 interpreterPath: process.execPath 的 fixture 都会复制、gzip、再下载并解包宿主机约 124 MB 的解释器二进制，而这一打包开销约占本文件运行时间的 99%。五个 serveArchive 调用点经 it.each 展开成 17 个这样的用例，每个约写入 380 MB（复制 + 归档 + readFileSync + Uint8Array 拷贝 + 下载 + 解包）。
具体代价是：本地每次运行、CI 每个分片都要多花约 66 秒和约 6.5 GB 的临时 I/O，而本仓库的 CI 已经记录过这些 runner 池的磁盘压力和负载尖峰。其余 119 个用例加起来不到半秒。
实测：未修改的树上连续两次全文件运行均为 136 通过、约 66.7/66.8 秒；从 junit 报告解析出的分布显示 17 个用例各约 3.9 秒、合计 66.37 秒（占 99.3%），另 119 个合计 0.45 秒。应用候选修复（用 #!/bin/sh\nexec '<execPath>' "$@" 包装脚本替代 copyFileSync）后为 1.38 秒（48 倍），136/136 仍全绿；反向对照（把包装脚本换成不执行的 printf "1.2.3\n" 假解释器）则有 17 个失败，且正是那 17 个慢用例——这证明 exec 包装保留了这些用例依赖的真实运行时行为，而不是把它们悄悄变空。
决定哪些用例可以迁移的约束：五个 preload/redaction 用例依赖真实解释器响应注入的选项（:729-730 的 NODE_OPTIONS/BUN_OPTIONS stub，以及 :747-748 的 cliSource 守卫 process.exit(9)）。把它们指向只会 printf 的假解释器会让污染与脱敏断言变成空断言而不是变快，因此必须继续启动真实解释器，exec 包装满足这一点。仅在 Linux + Node 宿主上验证，17 个用例都是 skipIf(win32)，未覆盖 Bun 宿主。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| } | ||
| return { registry, authorization }; | ||
| } | ||
| if (authPath === '/') break; |
There was a problem hiding this comment.
[Suggestion] R1-23: The credential walk only ever builds trailing-slash keys (//host<path/>:), while npm also probes the slash-less forms and the bare //host.
A user whose .npmrc carries //registry.test:_authToken=... (no trailing slash) or //registry.test/npm:_authToken=... — both forms npm's own getAuth resolves — gets no Authorization header from this module. The update check then sends an unauthenticated request to a private registry and reports a 401 or 'no update available', with nothing indicating that a valid credential was present but unmatched.
Witness:
Differential against the real npm-registry-fetch/lib/auth.js (getAuth),
same config, same URI:
//registry.test:_authToken=dummy , registry https://registry.test/
PR module -> { registry: 'https://registry.test/', authorization: undefined }
npm -> {"regKey":"//registry.test","authKey":"_authToken","token":"dummy"}
//registry.test/npm:_authToken=dummy , registry https://registry.test/npm
PR module -> authorization: undefined
npm -> {"regKey":"//registry.test/npm","authKey":"_authToken","token":"dummy"}
POSITIVE CONTROL //registry.test/:_authToken=dummy
PR module -> authorization: 'Bearer dummy'
(the comparator reports a difference only where one exists)
The authority's code, matching the claim:
let regKey = `//${parsed.host}${parsed.pathname}`
while (regKey.length > '//'.length) { ... }
regKey = regKey.replace(/([^/]+|\/)$/, '')
// comment: "can be either //host/some/path/:_auth or //host/some/path:_auth"
-> npm strips EITHER the last segment OR the slash, so from //registry.test/
it also probes //registry.test
This loop keys only `//${registry.host}${authPath}:` with authPath always
slash-terminated, and breaks at `authPath === '/'`.
Extend the walk to also probe the slash-less key at each level and the bare //host form, mirroring npm-registry-fetch's replace(/([^/]+|\/$)/, '') step. New cases asserting //registry.test:_authToken=dummy and //registry.test/npm:_authToken=dummy each resolve to Bearer dummy must go red when the added key forms are removed.
This was verified against the authority rather than by argument: npm's own walk on the existing sibling-path fixture (registry=https://registry.test/other/ with //registry.test/private/:_authToken and //other.test/:_authToken) returns {"regKey":false,"authKey":null,"token":null} — no match — so a npm-faithful slash-less probe keeps npm-registry-config.test.ts:195-200 green. The walk must stay longest-prefix-first per :177. Termination is already guaranteed: authPath strictly shortens and / breaks before the final slice, including // runs.
Note this is related to but distinct from R1-17, which is about the walk-up being untested rather than incomplete.
中文说明
凭据回溯只会构造带尾斜杠的键（//host<path/>:），而 npm 还会探测无斜杠形式以及裸 //host。
.npmrc 中写有 //registry.test:_authToken=...（无尾斜杠）或 //registry.test/npm:_authToken=... 的用户——这两种形式 npm 自己的 getAuth 都能解析——从本模块拿不到 Authorization 头。于是更新检查会向私有注册表发送未认证请求，并报告 401 或“无可用更新”，而没有任何提示说明其实存在有效凭据、只是没有匹配上。
实测：对真实的 npm-registry-fetch/lib/auth.js（getAuth）在相同配置与相同 URI 下做差分。//registry.test:_authToken=dummy 配 registry https://registry.test/：本模块得到 authorization: undefined，npm 得到 {"regKey":"//registry.test","authKey":"_authToken","token":"dummy"}；//registry.test/npm:_authToken=dummy 配 registry https://registry.test/npm：本模块 undefined，npm 正常解析。正向对照 //registry.test/:_authToken=dummy 下本模块得到 'Bearer dummy'，说明比较器只在确有差异时才报告差异。权威实现的代码与论断一致：let regKey = //${parsed.host}${parsed.pathname}``、while (regKey.length > '//'.length)、`regKey = regKey.replace(/([^/]+|/)$/, '')`，注释为“can be either //host/some/path/:_auth or //host/some/path:_auth”，即 npm 会剥掉最后一个路径段或那个斜杠，因此从 `//registry.test/` 也会探测 `//registry.test`。而本循环只用始终以斜杠结尾的 `authPath` 构造 `//${registry.host}${authPath}:`，并在 `authPath === '/'` 时 break。
建议扩展回溯，在每一级同时探测无斜杠键以及裸 //host 形式，对应 npm-registry-fetch 的 replace(/([^/]+|\/$)/, '') 步骤。新增用例断言 //registry.test:_authToken=dummy 与 //registry.test/npm:_authToken=dummy 各自解析为 Bearer dummy；移除新增的键形式时它们必须变红。
这一点是对照权威实现验证的，而非靠推理：npm 自身在现有兄弟路径 fixture（registry=https://registry.test/other/，token 只在 //registry.test/private/:_authToken 与 //other.test/:_authToken）上的回溯返回 {"regKey":false,"authKey":null,"token":null}，即不匹配，因此符合 npm 的无斜杠探测能让 npm-registry-config.test.ts:195-200 保持绿色。按 :177 的要求，回溯必须保持最长前缀优先。终止性已有保证：authPath 严格变短，且在最终 slice 之前遇到 / 就 break（包括 // 连续斜杠）。注意这与 R1-17 相关但不同：后者讲的是回溯未被测试，这里讲的是回溯不完整。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| for (const [index, manifest] of [ | ||
| null, | ||
| 123, | ||
| 'private manifest content', |
There was a problem hiding this comment.
[Suggestion] R1-24: The manifest-gate loop plants the sentinel 'private manifest content' as a manifest body, but no assertion ever reads it. The rejection is checked with error.message.startsWith(...) plus two assert.match(...) regexes — a shape that tolerates any suffix and also leaves the message's fourth sentence unpinned.
Because the harness only checks a prefix and two interior phrases, a change that appends content to that message — chaining the underlying failure the way R1-10 asks for on the sibling smoke-test paths, or interpolating the raw manifest bytes or the JSON.parse error whose Node text embeds a snippet of the input — ships green while the user-facing recovery error for a corrupted install starts echoing arbitrary on-disk content and/or silently drops the fourth recovery sentence.
The harness demonstrably knows the stricter shape: the real-interpreter failure loop at :775-793 plants the same kind of sentinel ('private child output' in the child's stdout AND stderr) and pins non-leakage with an exact { message } match, so appending child output there WOULD fail. The manifest loop is the one place where the canary has no reader.
Witness:
git grep -n 'private manifest content' HEAD -> exactly 2 hits repo-wide:
scripts/test-standalone-runtimes.mjs:827 (planted)
packages/cli/src/ui/standalone-update.test.ts:616 (planted)
readers: 0
The assertion block at :836-851 is:
error.message.startsWith('Invalid installed standalone manifest at ... ')
+ assert.match(/known-good backup/)
+ assert.match(/same runtime flavor in a new directory/)
-> no exact match
Mutation check by reading: interpolating raw manifest bytes into the
message satisfies startsWith, both regexes, AND the unit test's substring
check at standalone-update.test.ts:616 (rejects.toThrow(string) is substring
containment in vitest) - so the "appends/echoes raw on-disk content" half is
caught nowhere. The "drops the fourth sentence" half IS caught by the unit
test.
Assert the message exactly — the path is already computed in the loop — which pins all four sentences and forbids any appended content; or at minimum keep the existing matches and add assert.ok(!error.message.includes('private manifest content'), 'manifest content leaked into the error') so the planted sentinel has a reader. Mutation proof: change standalone-update.ts:1200 to append ${raw.slice(0, 64)} and re-run the harness; today all 14 gate cases stay green, and with the fix the index === 2 case must fail. The unit-test twin of this gap is filed as R1-6.
The expected string must stay byte-identical to the concatenation at standalone-update.ts:1199-1203, which is a single shared invalidManifestMessage thrown from both the JSON.parse catch and the non-object branch — so one exact-match expectation covers all five index < 5 cases without per-case strings.
中文说明
manifest 门禁循环把哨兵字符串 'private manifest content' 植入为 manifest 内容，但没有任何断言读取它。拒绝是通过 error.message.startsWith(...) 加两个 assert.match(...) 正则来检查的——这种形式容忍任意后缀，同时也让消息的第四句没有被固定。
由于 harness 只检查一个前缀和两个内部短语，任何给该消息追加内容的改动——例如像 R1-10 建议在同类 smoke 路径上串接底层错误，或插入原始 manifest 字节、或插入 JSON.parse 错误（Node 的错误文本会包含输入片段）——都会在全绿的情况下合入，而面向用户的“安装损坏”恢复提示会开始回显任意磁盘内容，和/或静默丢掉第四句恢复指引。
这个 harness 显然知道更严格的写法：:775-793 的真实解释器失败循环植入了同类哨兵（子进程 stdout 与 stderr 中的 'private child output'），并用精确的 { message } 匹配固定不泄漏，因此在那里追加子进程输出会失败。manifest 循环是唯一一个哨兵没有读取方的地方。
实测：git grep -n 'private manifest content' HEAD 全仓恰好 2 处命中，都是植入方（scripts/test-standalone-runtimes.mjs:827 与 packages/cli/src/ui/standalone-update.test.ts:616），读取方为 0。:836-851 的断言块是 startsWith 加两个 assert.match，没有精确匹配。通过阅读做变异检查：把原始 manifest 字节插入消息后，startsWith、两个正则、以及 standalone-update.test.ts:616 的子串检查（vitest 中 rejects.toThrow(string) 是子串包含）全部仍然满足，因此“追加/回显原始磁盘内容”这一半在任何地方都不会被发现；“丢掉第四句”这一半则会被单元测试发现。
建议精确断言该消息（路径在循环中已经算好），这样能固定全部四句并禁止任何追加内容；或至少保留现有匹配并加上 assert.ok(!error.message.includes('private manifest content'), 'manifest content leaked into the error')，使植入的哨兵有读取方。变异验证：把 standalone-update.ts:1200 改为追加 ${raw.slice(0, 64)} 并重跑 harness；今天 14 个门禁用例全部保持绿色，加上修复后 index === 2 那一例必须失败。该缺口在单元测试中的孪生问题见 R1-6。
约束：期望字符串必须与 standalone-update.ts:1199-1203 的拼接逐字节一致，那是由 JSON.parse catch 与非对象分支共同抛出的单一共享 invalidManifestMessage，因此一条精确匹配期望即可覆盖全部五个 index < 5 用例，无需逐例字符串。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| let node = process.env.PINNED_NODE; | ||
| let bun = process.env.PINNED_BUN; | ||
| if (!node) { | ||
| const filename = `node-v24.19.0-${process.platform}-${process.arch}.tar.gz`; |
There was a problem hiding this comment.
[Suggestion] R1-25: The fixture's runtime pins are bare literals decoupled from the release tooling that decides what actually ships — and the Node pin has already diverged. This script downloads and hard-asserts Node v24.19.0, while shipped node-flavor standalone archives embed process.versions.node of the release lane, which is Node 22.x.
So the regression harness that exists to prove real pinned interpreters survive a standalone update exercises a Node major that is never shipped. A Node-22-vs-24 difference in the exact machinery under test — execPath/execFile argv resolution, the launcher spawn path, NODE_EXTRA_CA_CERTS handling, or the rename/replace semantics behind atomicReplace — passes green here and ships broken in the 22.x archive. The assert.equal(run(node, ['--version']), 'v24.19.0') actively rejects retargeting the fixture at the real runtime via PINNED_NODE. The Bun half aligns only by duplicated literals against DEFAULT_BUN_VERSION, so bumping that constant silently desynchronizes the fixture.
Witness:
.nvmrc -> 22
build-standalone-release.js:111 -> const nodeVersion =
args.nodeVersion || process.versions.node; (no DEFAULT_NODE_VERSION)
package.json:124 -> "package:standalone:release":
"node scripts/build-standalone-release.js" (no --node-version)
run-release-step.sh:175 -> passes only --version,
--out-dir, ${preview_args}
release.yml:197,276,659 -> node-version-file: '.nvmrc'
release.yml:200 "ECS uses its pre-installed Node because nodejs.org may
be unreachable" + uses: './.github/actions/self-hosted-node'
-> on the self-hosted release lane the embedded Node is not even
.nvmrc-pinned; there is no single source of truth
ci.yml -> 5x node-version: '22.x',
2x node-version-file: '.nvmrc'
git grep -n '24\.19\.0' -> 4 hits in this harness
(:927, :930, :1005, :38), 5 elsewhere, 0 in any release path
Bun asymmetry: DEFAULT_BUN_VERSION = '1.3.14' (build-standalone-release.js:65)
matches the harness, and the repo CITES it as the convention:
tui-parity.yml:47,101 "Pinned to DEFAULT_BUN_VERSION
(scripts/build-standalone-release.js,"
e2e.yml:567 "DEFAULT_BUN_VERSION in scripts/build-standalone-
release.js (the"
This script carries no such citation.
Derive from the release tooling instead of restating it: export DEFAULT_BUN_VERSION from scripts/build-standalone-release.js and import it here; add a DEFAULT_NODE_VERSION there (or derive the expectation from .nvmrc) and pin the fixture to it; then replace all four Node literals and all three Bun literals with the constants. If Node 24 is a deliberate forward-compatibility choice rather than drift, say so in a comment at :927 and add a second matrix leg for the shipped 22.x, since a pin that is never shipped provides no coverage of the shipped artifact. If the retarget is taken, :1005 is itself the witness — it must go red if the pin is set back to a version the download site does not fetch.
scripts/build-standalone-release.js:65's const DEFAULT_BUN_VERSION = '1.3.14'; is the canonical Bun pin and is already cited as such by two workflows, so the fix must derive from it rather than restate it. For Node, build-standalone-release.js:111 means there is no constant to import, so the fix must not assume one exists.
One correction to the original report: the claim that 24.19.0 appears nowhere else in the repo is false — this PR's own design docs cite it (bun-runtime-boundary.md:300 and .zh-CN.md:212). The divergence chain above is nonetheless correct, and the design doc states the pin without justifying a major the release lane never ships; its 'Regression evidence and limits' section lists several other limits but not this one.
中文说明
fixture 的运行时版本钉是裸字面量，与真正决定发布内容的发布工具解耦——而 Node 的钉已经发生偏离。本脚本下载并硬断言 Node v24.19.0，而实际发布的 node flavor standalone 归档内嵌的是发布流水线的 process.versions.node，也就是 Node 22.x。
因此，这个用来证明“真实且固定版本的解释器能在 standalone 更新中存活”的回归 harness，实际演练的是一个从不发布的大版本。在被测的这套机制中——execPath/execFile 的 argv 解析、launcher spawn 路径、NODE_EXTRA_CA_CERTS 处理、atomicReplace 背后的重命名/替换语义——任何 Node 22 与 24 的差异都会在这里全绿通过，然后在 22.x 归档中带着问题发布。assert.equal(run(node, ['--version']), 'v24.19.0') 还主动阻止通过 PINNED_NODE 把 fixture 指向真实运行时。Bun 那一半只是靠重复字面量与 DEFAULT_BUN_VERSION 巧合一致，因此提升该常量会静默地让 fixture 失同步。
实测：.nvmrc 为 22；build-standalone-release.js:111 是 const nodeVersion = args.nodeVersion || process.versions.node;（不存在 DEFAULT_NODE_VERSION）；package.json:124 调用时不带 --node-version；run-release-step.sh:175 只传 --version、--out-dir、${preview_args}；release.yml:197,276,659 使用 node-version-file: '.nvmrc'；release.yml:200 注明“ECS uses its pre-installed Node because nodejs.org may be unreachable”并使用 ./.github/actions/self-hosted-node，即在自托管发布线上内嵌的 Node 甚至不受 .nvmrc 约束，根本不存在单一事实来源；ci.yml 有 5 处 node-version: '22.x' 与 2 处 .nvmrc。git grep -n '24\.19\.0' 在本 harness 中有 4 处命中（:927、:930、:1005、:38），其余 5 处，发布路径中 0 处。Bun 侧的不对称：DEFAULT_BUN_VERSION = '1.3.14'（build-standalone-release.js:65）与 harness 一致，且仓库把它当作约定来引用（tui-parity.yml:47,101、e2e.yml:567），而本脚本没有这样的引用。
建议从发布工具派生而不是重述：从 scripts/build-standalone-release.js 导出 DEFAULT_BUN_VERSION 并在此导入；在那里新增 DEFAULT_NODE_VERSION（或从 .nvmrc 推导期望值）并把 fixture 钉到它；然后用常量替换全部四个 Node 字面量与三个 Bun 字面量。如果 Node 24 是有意的前向兼容选择而非漂移，请在 :927 加注释说明，并为实际发布的 22.x 增加第二条矩阵分支，因为从不发布的版本钉无法覆盖已发布的产物。若采纳重定向，:1005 本身就是见证——把钉改回下载站不会提供的版本时它必须变红。
约束：scripts/build-standalone-release.js:65 的 const DEFAULT_BUN_VERSION = '1.3.14'; 是权威的 Bun 版本钉，且已被两个 workflow 引用，因此修复必须从它派生而不是重述；对 Node 而言，build-standalone-release.js:111 意味着没有可导入的常量，修复不能假定它存在。
对原报告的一处更正：所谓 24.19.0 在仓库其他地方完全不出现是不成立的——本 PR 自己的设计文档引用了它（bun-runtime-boundary.md:300 与 .zh-CN.md:212）。不过上述偏离链条依然正确，而且设计文档只陈述了该版本钉、没有论证为何选择一个发布线从不发布的大版本；其“Regression evidence and limits”一节列举了若干其他限制，却没有这一条。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| if (!node) { | ||
| const filename = `node-v24.19.0-${process.platform}-${process.arch}.tar.gz`; | ||
| const base = | ||
| process.env.QWEN_TEST_NODE_BASE_URL || |
There was a problem hiding this comment.
[Suggestion] R1-26: The Node leg's checksum verification is self-referential — SHASUMS256.txt and the tarball are both fetched from the same base, so a mirror attests its own artifact — and its trust anchor is an environment variable documented nowhere, while the Bun leg 30 lines below pins a hardcoded origin. The TLS hardening is asymmetric too: ci()'s own download() runs in the inherited environment with no NODE_TLS_REJECT_UNAUTHORIZED delete, while the worker logs tlsVerification: true.
A lane that exports QWEN_TEST_NODE_BASE_URL to a mirror, or NODE_TLS_REJECT_UNAUTHORIZED=0, performs the provisioning fetches with no meaningful integrity or certificate validation. A modified node binary plus a regenerated SHASUMS256.txt satisfies every remaining guard (https: protocol, no credentials, trailing slash), and that binary then runs the entire regression with the runner's credentials and $TMPDIR. Meanwhile the design doc this PR adds states CI mode 'verifies published checksums/integrity', which is untrue for such a lane, and nothing in the harness output says the override is active.
Witness:
git grep -n QWEN_TEST_NODE_BASE_URL HEAD
-> exactly ONE hit, :929 itself. No workflow, no design doc, no .env docs.
It is live whenever PINNED_NODE is unset, which is precisely the
--ci path.
git grep -n NODE_TLS_REJECT_UNAUTHORIZED (in this harness)
-> ONE hit, :1035, inside the non-ci entry's WORKER spawn env.
ci()'s own download() has no such delete, while the worker logs
`tlsVerification: true` at :875.
Self-attestation, quoted from ci() at :927-946:
expected is parsed from download(base + 'SHASUMS256.txt')
bytes come from download(base + filename)
-> same origin. Remaining guards: assert.equal(source.protocol, 'https:'),
assert.equal(source.username + source.password, ''),
assert(base.endsWith('/'))
Bun leg asymmetry, 30 lines below:
assert.equal(new URL(metadata.dist.tarball).origin, 'https://registry.npmjs.org')
sha512-... checked against metadata.dist.integrity from the packument
Make the Node leg's integrity non-self-attesting — pin the expected sha256 for the pinned version as a literal beside the pin, or fetch SHASUMS256.txt from a fixed trusted origin independent of the mirror override — and emit a loud one-line warning whenever QWEN_TEST_NODE_BASE_URL or NODE_TLS_REJECT_UNAUTHORIZED alters the trust basis, so the harness output cannot claim verified integrity it did not establish. Document the override in the design doc's CI-mode paragraph. A case in a new scripts/tests/test-standalone-runtimes.test.js (the repo already parses source and workflow text in scripts/tests/*.test.js) should assert the Node leg carries a pinned-digest literal or an origin assertion symmetric with the Bun leg, and that the warning is emitted when either override is set; it must go red if the added alternative is removed.
release.yml:200's comment 'ECS uses its pre-installed Node because nodejs.org may be unreachable' makes restricted-egress lanes a real, already-acknowledged condition, so the override must keep working — the fix is a pinned digest or a loud warning, not removal of the override.
中文说明
Node 分支的校验和验证是自指的——SHASUMS256.txt 与 tarball 都从同一个 base 获取，因此镜像在为它自己的产物作证——而它的信任锚是一个在任何地方都没有文档的环境变量；相比之下 30 行之下的 Bun 分支钉死了硬编码来源。TLS 加固也是不对称的：ci() 自己的 download() 运行在继承的环境中，没有删除 NODE_TLS_REJECT_UNAUTHORIZED，而 worker 却记录 tlsVerification: true。
一个把 QWEN_TEST_NODE_BASE_URL 指向镜像、或设置 NODE_TLS_REJECT_UNAUTHORIZED=0 的流水线，会在没有实质完整性或证书校验的情况下完成下载。被修改过的 node 二进制加上重新生成的 SHASUMS256.txt 能满足其余所有守卫（https: 协议、无凭据、尾斜杠），而这个二进制随后会带着 runner 的凭据和 $TMPDIR 跑完整个回归。同时，本 PR 新增的设计文档声称 CI 模式“verifies published checksums/integrity”，对这样的流水线并不成立，而 harness 的输出也没有任何地方说明该覆盖已生效。
实测：git grep -n QWEN_TEST_NODE_BASE_URL HEAD 恰好一处命中，即 :929 本身，没有任何 workflow、设计文档或 .env 文档提及；只要 PINNED_NODE 未设置它就生效，而这正是 --ci 路径。git grep -n NODE_TLS_REJECT_UNAUTHORIZED（本 harness 内）只有一处命中 :1035，位于非 ci 入口的 worker spawn env 中；ci() 自己的 download() 没有该删除，而 worker 在 :875 记录 tlsVerification: true。自指部分引自 :927-946 的 ci()：expected 解析自 download(base + 'SHASUMS256.txt')，bytes 来自 download(base + filename)，来源相同；其余守卫为 assert.equal(source.protocol, 'https:')、assert.equal(source.username + source.password, '')、assert(base.endsWith('/'))。Bun 分支的不对称在 30 行之下：assert.equal(new URL(metadata.dist.tarball).origin, 'https://registry.npmjs.org')，并用 packument 的 metadata.dist.integrity 校验 sha512-...。
建议让 Node 分支的完整性不再自证——把固定版本对应的期望 sha256 作为字面量钉在版本钉旁边，或从独立于镜像覆盖的固定可信来源获取 SHASUMS256.txt——并在 QWEN_TEST_NODE_BASE_URL 或 NODE_TLS_REJECT_UNAUTHORIZED 改变信任基础时输出一行醒目警告，使 harness 的输出不能声称自己并未建立的完整性验证。同时在设计文档的 CI 模式段落中记录该覆盖变量。可在新建的 scripts/tests/test-standalone-runtimes.test.js 中加用例（仓库已在 scripts/tests/*.test.js 中解析源码与 workflow 文本），断言 Node 分支带有固定摘要字面量或与 Bun 分支对称的来源断言，并且在任一覆盖被设置时会发出警告；移除新增的分支时它必须变红。
约束：release.yml:200 的注释“ECS uses its pre-installed Node because nodejs.org may be unreachable”说明受限出口的流水线是真实且已被承认的情况，因此该覆盖必须继续可用——修复应是固定摘要或醒目警告，而不是移除覆盖。
— qwen3.8-max via Qwen Code /review (v0.25.3)
| const result = spawnSync(node, [script], { | ||
| env: { ...process.env, PINNED_NODE: node, PINNED_BUN: bun }, | ||
| stdio: 'inherit', | ||
| timeout: 600_000, |
There was a problem hiding this comment.
[Suggestion] R1-27: The --ci parent's 600 s budget is exactly the sum of the two sequential per-host worker budgets (2 x 300 s at :1052), leaving zero slack for the child's own setup — four run() version probes plus openssl req -x509, each of which also inherits run's 300 s default, for a full worst case of about 2100 s against a 600 s cap. When it trips, the parent's SIGTERM always beats the child's diagnosable per-step timeout.
On a loaded self-hosted runner the outer cap is reached first and spawnSync signals only the child PID. Two losses are confirmed: the child dies before it replays the worker's buffered transcript at :1057, so every JSON line the worker emitted — including its failurePhase diagnostic at :891 — is discarded and CI reports a bare expected/actual SIGTERM assertion naming no runtime, scenario or phase; and the child's finally { fs.rmSync(root, ...) } at :988 never runs, leaving the fixture root behind on a lane whose own comments document prior ENOSPC and /tmp-inode pressure. Because package.json chains this ahead of the scripts suite with &&, the kill also reddens test:ci and test:release for whatever unrelated PR happened to be running.
Witness:
Constants read off the real file:
:23 run's default timeout: 300_000
:977-981 --ci parent spawnSync(node, [script], { stdio: 'inherit',
timeout: 600_000 })
:1052 each worker spawnSync(bin, [script, '--worker', ...],
{ timeout: 300_000 })
-> outer (600 s) == 2 x inner (300 s), zero slack
Wall time MEASURED on the real fixture rather than modelled:
{"host":"node","actualExitCode":0,"signal":null,"errorCode":null,
"elapsedMs":254191}
-> 254 s = 85% of its own 300 s allowance, and 42.4% of the parent's
entire 600 s budget consumed by the FIRST of two legs, on a loaded box
Cleanup loss confirmed on the real script: after SIGTERM to the child PID,
child temp root still present = yes, size = 744M,
sockets still owned by the worker = 0
CORRECTION: the orphaned-grandchild claim was REFUTED by the same
measurement (grandchild alive = no, 0 listening sockets) - do not act on it.
Give the outer budget real headroom over the inner ones so the inner timeout — which reports and cleans up — always wins the race: read both from one place (a single WORKER_TIMEOUT_MS used at :1052) and set the outer to 2 * WORKER_TIMEOUT_MS + 120_000, or drop the parent timeout entirely and let the per-worker and per-run budgets be the binding constraint. Note that adding detached: true alone does NOT fix anything, because spawnSync's timeout signals the child PID rather than the process group.
scripts/tests/ has no coverage of this harness today, so the pinnable invariant is the bound relationship: a new case in scripts/tests/package-scripts.test.js asserting the outer timeout is strictly greater than twice the per-host worker timeout goes red if the constants are re-equalised. N/A if the fix only reduces per-leg work.
.github/workflows/ci.yml:402's timeout-minutes: 120 is the test job's own ceiling, shared with test:ci:workspaces, and release.yml:431-434 gives the quality_scripts job timeout-minutes: 30, so raising the outer cap past roughly 1500 s moves the failure from the script to the job timeout rather than removing it. scripts/tests/no-ak-integration-ci.test.js:130-155 pins the ordered command sequence inside the test/test_windows steps byte-for-byte, so a workflow-side variant must preserve it. Related, measured in the same run: the inner 300 s per-worker budget itself has only ~46 s of headroom on a loaded lane (254,191 ms of 300,000 ms), so both constants sit near their measured wall time — but only the outer one loses evidence.
中文说明
--ci 父进程的 600 秒预算恰好等于两个顺序执行的按主机 worker 预算之和（:1052 处 2 x 300 秒），没有为子进程自身的准备留任何余量——四次 run() 版本探测加 openssl req -x509，而它们每一个也都继承 run 的 300 秒默认值，因此完整最坏情况约为 2100 秒，对应 600 秒的上限。一旦触发，父进程的 SIGTERM 总会先于子进程可诊断的分步超时。
在负载较高的自托管 runner 上，外层上限会先被触及，而 spawnSync 只对子进程 PID 发信号。已确认两处损失：子进程在 :1057 回放 worker 缓冲输出之前就死了，因此 worker 发出的每一行 JSON——包括 :891 的 failurePhase 诊断——都被丢弃，CI 只报告一个裸的 expected/actual SIGTERM 断言，不说明运行时、场景或阶段；并且子进程 :988 的 finally { fs.rmSync(root, ...) } 从未执行，于是 fixture 根目录被遗留在这条自身注释已记录过 ENOSPC 与 /tmp inode 压力的流水线上。由于 package.json 用 && 把它串在 scripts 套件之前，这个 kill 还会让当时正在运行的任何无关 PR 的 test:ci 与 test:release 变红。
实测：从真实文件读出的常量——:23 的 run 默认 timeout: 300_000；:977-981 的 --ci 父进程 spawnSync(node, [script], { stdio: 'inherit', timeout: 600_000 })；:1052 的每个 worker spawnSync(bin, [script, '--worker', ...], { timeout: 300_000 })，即外层（600 秒）等于 2 倍内层（300 秒），余量为零。在真实 fixture 上测量（而非建模）得到的墙钟时间：{"host":"node","actualExitCode":0,"signal":null,"errorCode":null,"elapsedMs":254191}——254 秒，占其自身 300 秒额度的 85%，并且两条腿中的第一条就消耗了父进程整个 600 秒预算的 42.4%（在负载较高的机器上）。清理损失也在真实脚本上确认：对子进程 PID 发 SIGTERM 后，子进程临时根目录仍存在、大小 744M，worker 仍持有的套接字为 0。更正：关于孙进程成为孤儿的说法已被同一次测量否证（孙进程存活为否，监听套接字 0 个），不要据此行动。
建议让外层预算相对内层有真实余量，使会报告并清理的内层超时总是赢得竞争：把两者放在同一处定义（在 :1052 使用单一的 WORKER_TIMEOUT_MS），并把外层设为 2 * WORKER_TIMEOUT_MS + 120_000；或者干脆去掉父进程超时，让按 worker 与按 run 的预算成为约束。注意单独加 detached: true 并不能解决问题，因为 spawnSync 的超时是对子进程 PID 而非进程组发信号。
今天 scripts/tests/ 没有覆盖这个 harness，因此可固定的不变量是二者的大小关系：在 scripts/tests/package-scripts.test.js 中新增用例，断言外层超时严格大于按主机 worker 超时的两倍；常量被重新设成相等时它会变红。如果修复只是减少每条腿的工作量，则此项为 N/A。
约束：.github/workflows/ci.yml:402 的 timeout-minutes: 120 是 test job 自身的上限，与 test:ci:workspaces 共享；release.yml:431-434 给 quality_scripts job 的是 timeout-minutes: 30，因此把外层上限提高到约 1500 秒以上只是把失败从脚本转移到 job 超时，而不是消除它。scripts/tests/no-ak-integration-ci.test.js:130-155 逐字节固定了 test/test_windows 步骤内命令的顺序，因此 workflow 侧的变体必须保持它。相关的一点（同一次运行中测得）：内层每 worker 300 秒预算本身在负载较高的流水线上也只有约 46 秒余量（300,000 毫秒中的 254,191 毫秒），所以两个常量都接近其实测墙钟时间——但只有外层那个会丢失证据。
— qwen3.8-max via Qwen Code /review (v0.25.3)
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approving at head d41362f40f3e5dbbce192dc5f610a28dad088fb4.
Historical blocking issues — none exist
The only review round is qwen-code-review-bot COMMENTED at this exact head, and its ledger carries 27 findings of which every one is sev:"S" — there is no sev:"C" entry, no CHANGES_REQUESTED and no inline [Critical] anywhere in this PR's history. Suggestions do not gate this verdict, so I did not spend budget adjudicating them.
Independent Critical-only scan — no blocking finding
I read all four production surfaces in full. The consistent property is that every mismatch fails closed: a wrong or unverifiable artifact aborts the update rather than installing it, which is the right direction for a self-update path.
updateCheck.ts (+63/−7) — the new native registry path cannot leak the npm credential. The Bun-standalone branch replaces npm view with a direct dist-tags fetch, which means this PR newly puts a user's registry token into application code, so the redirect handling is the load-bearing part:
redirect: 'manual'keeps redirect following in this code rather than the agent, which is what makes credential stripping possible at all.if (next.origin !== url.origin) auth = undefined;drops theAuthorizationheader on any cross-origin hop, andauthis only ever assignedundefinedafterwards — never restored — so stripping is monotonic. A redirect chain that leaves the registry and later returns cannot re-attach the token.- The origin comparison includes protocol, so an
https:→http:downgrade on the same host also drops the credential. next.protocolis allowlisted tohttps:/http:and embedded userinfo (next.username || next.password) is rejected, blockingfile:/gopher:targets and credential smuggling through the Location header.- The loop is bounded (
redirects === 5throws, so at most six requests),await response.body?.cancel()releases each intermediate body so sockets do not pile up, and a singleAbortSignal.timeout(FETCH_TIMEOUT_MS)caps the whole chain rather than each hop. - Response validation refuses a non-object/array body and a non-string tag, and an absent tag yields
latest ?? currentVersion— i.e. "no update", not a bogus one. classifyUpdateCheckErrorgainserror.name === 'TimeoutError', which is whatAbortSignal.timeout()rejects with, so this path reports as a timeout instead of an unclassified failure.
The branch is gated on isBunRuntime() && getInstallationInfo('', false).isStandalone, so the Node path is untouched — the boundary is preserved rather than widened.
npm-registry-config.ts (+173, new) — credentials are resolved conservatively and never surface in an error. Registry resolution follows npm's precedence (@scope:registry → registry → the public default), the merged tier order [...builtin, ...global, ...user, ...env] puts env last as npm does, and the comment records the deliberate exclusion of the project .npmrc to match the previous npm view --global behaviour — so this does not silently change which registry an existing user hits. The URL is normalised (trailing slash forced, search and hash cleared) before the credential key is built, so a query string cannot end up inside a //host<path>: lookup, and the path walk terminates at / rather than looping. Both failure paths throw fixed strings — Invalid npm registry configuration and Invalid npm registry authentication configuration — carrying neither the token nor the URL. The new Headers({ Authorization: authorization }) probe inside try is a real guard: a token containing CR/LF would throw there rather than being injected into the request header.
standalone-update.ts (+222/−47) — the runtime boundary is now enforced by measurement, not assumption. archiveFilename takes the runtime and appends -opentui-preview for bun, so a Bun install fetches the Bun archive; standaloneUpdateTarget reads runtime from the installed manifest, defaults to 'node', and throws on an unrecognised value instead of falling back — no silent downgrade to the wrong flavor. The manifest parse is now wrapped so a corrupt file yields a recovery instruction that explicitly tells the user to keep the installation and backup, rather than a bare SyntaxError.
The rewritten smokeTest is the part I examined most closely, because it is the last gate before an install is committed:
- The interpreter path is derived from
runtimeinstead of a hardcodednode, matching the archive actually downloaded. - It asserts the shipped interpreter's self-reported identity —
process.versions.bun ? 'bun' : 'node', plusprocess.platformandprocess.archagainst the parsedtarget, withwinmapped towin32— so an archive containing the wrong runtime or the wrong architecture fails here rather than after the swap. - It adds
semver.eq(version, expectedVersion)inside atrythat resolves tofalseon a parse error, so a stale or mismatched archive cannot pass on a well-formed but wrong version. - Probes run in a fresh
mkdtempdirectory (0700) with the reason stated: neither the archive's nor the user's projectbunfig.tomlpreloads are picked up. That matters because the probe executes a just-downloaded binary, and inheriting the user's project directory would let a preload run during the smoke test. spawnAndCapturescrubsNODE_OPTIONS,BUN_OPTIONS,CLI_VERSIONandQWEN_CODE_RELAUNCH_ARGSfrom the child environment so an inherited startup override cannot change what the probe measures, and addskillSignal: 'SIGKILL'so a hung probe is actually reaped on timeout.- The temp directory is removed in a
finally, with a warning rather than a throw if removal fails. - For
runtime === 'bun'an extra probe loadslib/cli-entry.js --help, so the launcher and not just the interpreter is verified.
maxBuffer: 64 * 1024 is new on these probes. If --help output ever exceeded it the child would be killed and the smoke test would fail — an aborted update, not a broken install, so the failure mode is safe.
package.json (+3/−1) adds the two test:standalone:runtimes scripts and chains the --ci variant ahead of the existing scripts suite with &&. No shipped dependency or entry-point change.
Scope disclosure — what I did not read. I did not audit scripts/test-standalone-runtimes.mjs (+1074, new), the four test files (standalone-update.test.ts +883/−13, npm-registry-config.test.ts +270, updateCheck.test.ts +224, hosted-harness-session.test.ts +80/−65), or the two design-doc additions. Those are gate and test infrastructure rather than shipped product code, so a defect there would surface as a false green rather than a user-facing fault. I am asserting no provable Critical in what I read, not the absence of one in what I did not.
CI
Fully green at this head — 28 pass, 41 skipped, nothing failing and nothing pending. Every lane that could exercise this change has reported.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
APPROVE at head d41362f4 (head SHA and CI re-read immediately before submitting; no failure, no cancelled instance in the current view, nothing pending).
Historical items. The review round at this same head posted 27 findings and every one is Suggestion-severity — there is no Critical among them, and the round was filed as a comment rather than a request for changes. Triage held confidence at 3/5 for two reasons, and both are closed at this head: the job that would prove the new harness works had not reported (it has — the Ubuntu workspace/scripts job ran the standalone matrix ahead of the scripts suite with both hosts exiting 0, Node 24.19.0 in 232.984 s and Bun 1.3.14 in 224.667 s, and the scripts suite itself reported 3,316 passed), and the publish-gating tradeoff was not written down (Risk & Scope now carries it in both languages: download and tool dependencies, the exact pins, fail-on-provisioning behavior, the Windows exclusion, and the observed 7 m 48 s cost).
Independent check. I read the security-sensitive surfaces myself rather than relying on the two prior rounds.
npm-registry-config.ts: credential lookup is bound to the resolved registry host and walks the path upward in the//host/path/shape npm uses, so a token for another host can never be attached; a registry URL carrying embedded userinfo is rejected outright; the protocol is restricted to http/https; the assembled header is validated throughnew Headers, which is what keeps a CR or LF inside an npmrc value from becoming a header injection; every throw uses a fixed message with no interpolated config value, so no credential can leave through error text; and an unmatched lookup returns no authorization at all, which fails closed.updateCheck.ts: the native-fetch path is gated on a standalone Bun install and everything else still usesnpm view --global; redirects are handled manually with a hard bound of five hops, the body is cancelled before following, a missing or unparsable location and any non-http(s) or userinfo-bearing target all reject, andnext.origin !== url.origindrops the Authorization header, so a cross-origin redirect cannot carry the credential. Response shapes are guarded (non-array object, string version) and an emptynpm viewstdout is still treated as no update.standalone-update.ts: the smoke test now asks the candidate binary what it is — it runs the interpreter with a one-line probe and rejects when the reported runtime, platform or arch disagree with the expected target — instead of trusting the manifest or the install path, which is the actual boundary this PR is about. The manifest read defaults an absentruntimeto node and rejects only values outside {node, bun}, so a legitimate field update cannot be refused by the new field, and archive naming plus interpreter paths are runtime-specific on both the updater and the packaging side.
CI. Every substantive lane is complete and green at this head: Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), Serve A/B, TUI parity snapshots, OpenTUI no-flicker gate, Native Linux CLI boundary, Native Windows settings and streams, web-shell E2E Smoke and Managed Agent fast tier. The macOS and Windows Test lanes are skipped by the workflow filters, so the win32 branches in the new smoke test and tilde handling are reasoned rather than executed — the author discloses the same limit.
Recorded for a maintainer, non-blocking. The one judgement call in this PR is operational rather than a defect: test:scripts now chains test:standalone:runtimes:ci ahead of the vitest suite with &&, and test:scripts feeds the release publish gate. So publishing now depends on egress to nodejs.org and registry.npmjs.org plus openssl, on two exact version pins that PINNED_NODE / PINNED_BUN can pre-provision but not change, and on a 1,074-line harness as the only place to edit them; a chained network failure also means roughly 115 scripts test files report nothing while the lane shows only a download error, and npm run preflight now pulls about 100 MB. The author states the intent explicitly — a provisioning failure should stay visible rather than produce an apparent pass — and that is a defensible call, but it is a release-gate policy and it belongs to whoever cuts releases. Worth tracking alongside it: the remaining Suggestions, of which the ones I would not let age are the untested redirect and response-shape branches, the credential walk only building trailing-slash keys, ${VAR} interpolation applying to values but not keys, the raw child-process error replaced by a fixed message, and the harness budget arithmetic that leaves no slack over its two sequential legs.
中文：head d41362f4 已复核，批准（提交前重读 SHA 与 CI：无 failure、无 cancelled、无 pending）。历史项：同一 head 那轮共 27 条发现，全部为 Suggestion 级，其中没有 Critical，且该轮以 comment 而非 request-changes 形式提交；triage 把信心压在 3/5 的两点在该 head 均已关闭——能证明新 harness 可用的 job 已经出结果（Ubuntu workspace/scripts job 在 scripts 套件之前跑完 standalone 矩阵，两个 host 均 exit 0，Node 24.19.0 用时 232.984s、Bun 1.3.14 用时 224.667s，scripts 套件本身 3316 通过），以及发布门禁取舍当时未写入文档（现在中英两版 Risk & Scope 都写明了下载与工具依赖、精确版本锁、provisioning 失败即失败的行为、Windows 排除与实测 7m48s 成本）。独立核对：安全敏感的三处我自己读过——npm-registry-config.ts 的凭据查找绑定在解析出的 registry host 上并按 npm 的 //host/path/ 形式向上回溯，因此不可能把别的 host 的 token 带上；带内嵌 userinfo 的 registry URL 直接拒绝；协议限制为 http/https；组装出的 header 经 new Headers 校验，这正是阻止 npmrc 值里的 CR/LF 变成头注入的那一层；所有抛出都使用固定文案、不内插任何配置值，凭据无法经错误文本外泄；未匹配时不返回 authorization，属 fail-closed。updateCheck.ts 的原生 fetch 路径只在 standalone Bun 安装下启用，其余仍走 npm view --global；重定向手动处理且硬上限 5 跳，跟随前取消 body，location 缺失或不可解析、目标非 http(s) 或带 userinfo 一律拒绝，且 next.origin !== url.origin 时丢弃 Authorization，跨域跳转带不走凭据；响应形状有守卫，npm view 空输出仍按无更新处理。standalone-update.ts 的 smoke test 改为直接询问候选二进制本身——用一行探针运行解释器，并在其上报的 runtime/platform/arch 与预期目标不一致时拒绝——而不再相信 manifest 或安装路径，这正是本 PR 要守的边界；manifest 读取在 runtime 缺失时默认 node、只拒绝 {node, bun} 之外的值，因此新字段不可能误拒真实更新，归档命名与解释器路径在更新侧与打包侧都按 runtime 区分。CI：该 head 上主体 lane 全部完成且绿（Test ubuntu、Lint & Static、Integration no-AK、Serve A/B、TUI parity、OpenTUI no-flicker、Native Linux/Windows、web-shell E2E Smoke、Managed Agent fast tier）；macOS 与 Windows 的 Test lane 按 workflow 过滤跳过，因此新 smoke test 与 tilde 处理中的 win32 分支是推理而非实测，作者也披露了同一限制。留给维护者的非阻塞记录：本 PR 唯一的判断题属运维而非缺陷——test:scripts 现在用 && 把 test:standalone:runtimes:ci 串在 vitest 之前，而 test:scripts 又喂给发布门禁，于是发布开始依赖到 nodejs.org 与 registry.npmjs.org 的出网、依赖 openssl、依赖两个精确版本锁（PINNED_NODE/PINNED_BUN 只能预置同版本、不能改版本），且唯一的修改入口是一个 1074 行的 harness；串联的网络失败还会让约 115 个 scripts 测试文件什么都不报，lane 只显示一个下载错误；npm run preflight 现在会拉取约 100 MB。作者已明确表达意图——provisioning 失败应当可见，而不是产生表面通过——这个取舍站得住，但它属于发布门禁策略，应由发版的人认领。与之并列值得跟踪的是其余 Suggestion，其中我不希望被放旧的是：重定向与响应形状分支无测试、凭据回溯只构造带尾斜杠的键、${VAR} 插值只作用于值而不作用于键、原始子进程错误被固定文案替代，以及 harness 的预算算术相对两条串行长腿没有任何余量。
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the update boundary at d41362f4. The runtime flavor is carried from the installed manifest through archive selection, download validation and activation, and I found no merge blocker.
What I traced:
- A legacy Node manifest without
runtimestill resolves toqwen-code-<target>.tar.gz, and a Bun payload sitting in a Node-flavored install is rejected by the interpreter identity probe instead of being activated (standalone-update.ts:520-545). - A first-time migration still creates the install directory (
standalone-update.ts:1413) before the only writer needs it; extraction goes to a siblingextractDir, so nothing writes into the missing directory earlier. - The prepared-update revalidation tolerates
undefinedversion/identity on both sides, so migration does not false-throw, while a genuinely replaced directory does (standalone-update.ts:1400-1412). - The native fetch branch is reached only for Bun standalone; every other install type still goes through
npm view(updateCheck.ts:174)./-/package/%40qwen-code%2Fqwen-code/dist-tagsanswers 200 on the public registry, and the redirect loop dropsAuthorizationwhen the origin changes.
Scope matches the description: 11 files, and the hosted-harness-session.test.ts edit is the CI-exposed cancellation cleanup the body already explains.
One non-blocking note inline (P3).
| throw new Error('Invalid npm registry configuration'); | ||
| } | ||
| if ( | ||
| !['https:', 'http:'].includes(registry.protocol) || |
There was a problem hiding this comment.
[P3] Non-blocking. http: is accepted here, so a user whose config sets registry=http://host/ gets the Authorization value built by the _authToken / _auth walk below sent in cleartext. npm behaves the same way, so this is parity rather than a new regression — but since this path is new and the token can come from any tier, it might be worth rejecting non-https: registries (or dropping the header when the query URL is not https).
|
Consolidating the duplicate update PRs #14139, #14142 and #14210 into this existing review. Their implementation, tests and source provenance are preserved here. Closing those PRs transfers review ownership; it does not turn unresolved Suggestions or platform gates into a pass. #14139：4 Suggestions，0 Critical
Review 本身披露原verify/tmux跳过、reverse audit预算不全；R1当前独立build/定向/runtime/CI有新的明确证据，但不等于旧review变成全面批准。 #14142：1 Critical + 10 Suggestions
正式review另引用“重复npmrc resolver与core resolveNpmRegistry/getNpmAuthToken分歧”的既报Suggestion；本次11threads中没有该历史评论正文，review引用保留。归属R1 registry合同；当前不合并成新的通用resolver/跨core重构，也不声称解析器全面等同npm。 #14210没有GitHub正式review或review thread。精确ce67edab的版本一致性生产/测试/脚本增量在R1保留，另修wrapper metadata-only fastpath；可按absorbed关闭，不附加不存在的review批准。 #14134 and #14187 remain open: their additional interpreter call-site work is being classified for the existing distribution PR #14959. No source branch is deleted. |
356890b
R1 independent verification —
|
| Verification | Actual result |
|---|---|
| Author repository gates | Full build/typecheck/bundle, lint/format passed; 215 focused tests passed (136 standalone update + 48 update query + 31 npm config), no failures/skips |
| Author real-runtime matrix | Node 24.19.0 and Bun 1.3.14 hosts both exited 0, each exercising both Node/Bun payloads; eight broken-launcher rejections across host × payload × direct/prepared mode passed; both temporary-residue lists empty |
| Independent Node archive positive | Valid local archive activated; actual installed command before and after exited 0; the prior live tree became the exact rollback backup |
| Independent Node archive negative | Broken launcher rejected with Smoke test failed: launcher could not load CLI; live and .old SHA256/modes unchanged; original installed command still exited 0 |
| Independent npm controls | All six expected trusted-tier/auth-path controls passed against real npm 11.17.0 under Node 24.19.0 |
| Independent npm regressions | All seven original mismatch inputs now exactly matched that authority; candidate config ran under Bun 1.3.14 |
| Non-acceptance observations | Three empty-value cases recorded, not counted as passes or silently reclassified |
The seven closed comparisons were templated authentication key, templated scope key, global-config path trailing whitespace, user-config path trailing whitespace, token leading whitespace, bare-host auth key and slashless auth-path key. Inputs and assertions were unchanged from the failing baseline. The new source contains focused coverage for both npmrc/environment templates, whitespace normalization, longest-match precedence and sibling/foreign-host rejection.
The three observation results are explicit: empty and whitespace-only global-config paths caused the product to fail closed; real npm recorded global-tier EISDIR and continued with the user registry. Empty prefix selected the owned cwd-relative etc/npmrc in both implementations. Those measurements do not support inventing an unconditional fallback or claiming complete npm error-policy parity.
Both independent groups ran once, exited 0 and passed their post-run guards. Their 120-second worker and 5-second TERM/5-second KILL fallback budgets were unchanged; fallback signals were not needed. The launcher made six requests to the owned HTTPS server; all 16 npm authority child invocations used the real pinned config/auth modules and made no network requests. A separate fresh process check found all 22 captured PID/start identities gone; both scratch roots were deleted, the server closed and update staging was empty.
Exact input binding
- Frozen independent manifest:
fd2af5145f3763d35ae614dce50a0eca4b0c694c8feace0dc7807ccea0821fbf. - Guarded inputs: 33,258 SHA256/mode entries, including all 18,874 files in 25 built roots and all 1,939 npm authority files; source, inventories and modes matched before/after both groups.
- Tested bundle:
d61877cdf6a9ea963aefc89684172de863acb2526115ac065dd2e1d7dc910549. - Source-to-build mapping:
90d6c0c67e1a88458b9305ccb025bad28aa092228b5b3ed8e1de99b782a7cee4. Author build/bundle preceded the normal commit; the seven committed repair files match the frozen tested source exactly. This is not a claim of a fresh post-commit rebuild. - Final independent cleanup/closure receipt:
20a30cc257f8b448ccdc3a09bbca0b812d555e3ad76dfd904e121d9e8784514a. - Independent report:
d12405da7c37eb4813d47d68f0a298b4f4a64e03330ffd4711b5c874a2840118; summary:03e855fa024efee219747fd643c489a2cf1e9fc07a813e5ad2c8e6e1538bab71.
The raw reports are retained in the worktree under .qwen/e2e-tests/r1-review-repro-candidate-v3-bound-356890b/; author receipts are under .qwen/investigations/r1-review-repair-validation-v1/. These ignored local paths are evidence locations, not downloadable GitHub artifacts. CI artifacts for the new head must be assessed after that run finishes.
Limits and review status
These are macOS arm64 results with synthetic owned archives and actual interpreters/configuration modules. They do not establish official signed release installation, all platforms, external corporate transport, ordinary-command launcher spawning or complete startup after rollback. Node recorded 143 raw probes in the author matrix; Bun reported its raw observer unavailable, so its zero field is not a measured zero-probe/zero-allocation result. The author’s 471 sampled process identities and independent 22 identities are separate cleanup evidence, not test-case counts.
Two independent open-ended full-diff static passes found no new confirmed defect in the audited candidate. That is static evidence, not a claim that all review Suggestions or untested paths are closed. Remote d41362f CI/approvals remain historical; new 356890b CI is in progress and has not yet been accepted as passing. The separate 28-row disposition covers all original 27 Suggestions plus the later HTTP-auth P3.
中文说明
R1 独立验证 — 356890b743e142f4226cb86de3bd8c9b29a92baf
本报告不覆盖任何原始失败：d41362f 的行为反例和首次 cache 隔离夹具失败均保留。修复源码及独立复验结果与作者更广的矩阵分开记录。独立对照未涉及真实用户设置、生产 token、付费模型或外部 npm registry 请求。
| 验证 | 实际结果 |
|---|---|
| 作者仓库门禁 | 完整 build/typecheck/bundle、lint/format 通过；215 个定向测试通过（standalone update 136 + update query 48 + npm config 31），无失败/跳过 |
| 作者真实 runtime 矩阵 | Node 24.19.0、Bun 1.3.14 宿主均 exit 0，分别运行 Node/Bun payload；宿主 × payload × 直接/预下载激活共八个坏启动器拒绝案例通过，两宿主临时残留列表均空 |
| 独立 Node 归档正控 | 有效本地归档激活，实际安装命令前后均 exit 0；旧 live 成为精确回滚备份 |
| 独立 Node 归档负控 | 损坏启动器以 Smoke test failed: launcher could not load CLI 拒绝；live/.old SHA256/mode 不变，原安装命令仍 exit 0 |
| 独立 npm 正控 | 六个可信层级/鉴权路径对照均通过，authority 为 Node 24.19.0 下真实 npm 11.17.0 |
| 独立 npm 回归 | 原七个差异输入现均与该 authority 精确一致；候选配置代码运行在 Bun 1.3.14 |
| 非验收观察 | 三个空值案例单列观察，不计为通过，不静默改分类 |
闭合的七项依次为：鉴权键模板、scope 键模板、global 配置路径尾空白、user 配置路径尾空白、token 前导空白、裸 host 鉴权键、不带尾斜杠的鉴权路径键。输入和断言与原失败基线逐字一致。新增源码定向覆盖 npmrc/环境两种模板、空白标准化、最长匹配及兄弟路径/其他 host 拒绝。
三个观察结果明确保留：空或纯空白 global-config 路径使产品 fail closed；真实 npm 记录 global 层 EISDIR 后继续使用 user registry。空 prefix 在两实现中都选择自有 cwd 下的 etc/npmrc。这些观察不支持臆造统一 fallback，也不代表 npm 错误策略完全等价。
两独立组各运行一次、exit 0，并通过运行后守卫。120 秒 worker 和 TERM 5 秒/KILL 5 秒兜底预算未改；无需兜底信号。Launcher 向自有 HTTPS 服务发出六次请求；16 次 npm authority 子进程调用使用真实固定 config/auth 模块，不访问网络。另一次 fresh process 检查确认 22 个捕获 PID/start 身份均消失；两处临时根已删除，服务已关闭，更新 staging 为空。
精确输入绑定
- 独立冻结 manifest：
fd2af5145f3763d35ae614dce50a0eca4b0c694c8feace0dc7807ccea0821fbf。 - 守卫输入：33,258 条 SHA256/mode，包含全部 25 个构建根的 18,874 个文件及 1,939 个 npm authority 文件；两组前后源码、文件清单及 mode 一致。
- 已测 bundle：
d61877cdf6a9ea963aefc89684172de863acb2526115ac065dd2e1d7dc910549。 - Source-to-build 映射：
90d6c0c67e1a88458b9305ccb025bad28aa092228b5b3ed8e1de99b782a7cee4。作者 build/bundle 在正常提交前完成；七个已提交修复文件与冻结被测源码精确一致，不代表提交后重新构建。 - 独立最终清理/闭环 receipt：
20a30cc257f8b448ccdc3a09bbca0b812d555e3ad76dfd904e121d9e8784514a。 - 独立报告：
d12405da7c37eb4813d47d68f0a298b4f4a64e03330ffd4711b5c874a2840118；summary：03e855fa024efee219747fd643c489a2cf1e9fc07a813e5ad2c8e6e1538bab71。
原始报告保存在 worktree 的 .qwen/e2e-tests/r1-review-repro-candidate-v3-bound-356890b/，作者 receipts 在 .qwen/investigations/r1-review-repair-validation-v1/。这些 ignored 本地路径是证据位置，并非可从 GitHub 下载的 artifact；新 head 的 CI artifact 需待运行完成后另行审阅。
限制与 review 状态
以上是 macOS arm64、自有合成归档和真实解释器/配置模块结果，不能证明正式签名安装、全部平台、外部企业传输、普通命令启动器 spawn 或回滚后完整启动。作者矩阵中 Node 记录 143 个 raw probes；Bun 报告 observer 不可用，因此其零字段不能解读为实测零 probe/零 allocation。作者 471 个采样进程身份和独立 22 个身份分别用于清理证明，不是测试案例数。
两次独立 open-ended 全 diff 静审未发现新的已确认缺陷。这是静态证据，不等于全部 review 建议或未测路径闭环。远端 d41362f CI/批准仅为历史结果；356890b 的新 CI 进行中，尚未确认通过。另一个 28 行处置评论覆盖原 27 条 Suggestions 及之后的 HTTP-auth P3。
Review disposition at
|
| Item | Disposition | Current evidence / next scope |
|---|---|---|
| R1-1 | Maintainer decision | Keep the visible fail-on-provisioning gate for this revision. Network/tool dependencies, skipped downstream scripts on provisioning failure, and release-budget coupling remain disclosed; moving the gate or accepting offline skips needs release-owner agreement. |
| R1-2 | Deferred — not fixed | The request deadline still spans several waits; the hosted test was not changed in this repair. Earlier ordinary/fixed-delay cleanup results do not establish safety for the reviewer’s cumulative loaded-host schedule. Budget design remains a coordinated follow-up with R1-3. |
| R1-3 | Deferred — not fixed | The three remaining default-budget waits are unchanged. Do not describe all waits as converted to the host-aware budget. Keep a paired follow-up with R1-2 instead of extending this registry/launcher repair. |
| R1-4 | Deferred unit assertion; runtime coverage present | The shell stub still does not execute its launcher payload. Eight real-interpreter broken-launcher negatives now cover both flavors and direct/prepared activation, plus the independent Node negative, but this does not close the requested unit mutation/knob coverage. |
| R1-5 | Deferred — not fixed | Real-interpreter archive copying remains. Preserve its behavioral coverage; optimize fixture I/O separately with a genuine runtime positive/negative control, not a printf-only substitute. |
| R1-6 | Deferred — not fixed | The installed-manifest sentinel is still not explicitly asserted absent from the full error. Add exact/redacted error assertions together with R1-24 in a bounded test-hardening follow-up. |
| R1-7 | Deferred — native platform acceptance | The win/win32 mapping remains correct by source inspection, but the requested executable Windows identity regression has not been added or run. POSIX success is not a Windows pass. |
| R1-8 | Deferred — diagnostic pinning | Installation preservation remains tested; the downloaded non-object manifest rejection message is not newly pinned. Syntax-invalid JSON and non-object JSON need separate error expectations. |
| R1-9 | Deferred — spawn-error coverage | The fixed redacted spawn message and timeout/buffer options remain. No new reachable error-event, kill-signal or max-buffer mutation test was run; an EACCES fixture alone would exercise the callback branch. |
| R1-10 | Deferred — safe diagnostics | Raw stderr remains excluded from user-facing errors and is not newly logged. A follow-up must choose a safe diagnostic sink and test redaction together with R1-9/R1-18 rather than adding raw cause/output opportunistically. |
| R1-11 | Partially fixed; remaining path deferred | Both Node and Bun now load the production launcher with --help before activation. The original Node broken-launcher failure is independently closed; all eight real-runtime rejection cases pass. The ordinary-command child-spawn branch is explicitly outside this probe and remains deferred. |
| R1-12 | Deferred — diagnostic consistency | Unknown runtime still fails closed with the existing generic message. Path/recovery wording harmonization is not implemented; the malformed-JSON/non-object recovery guidance remains the narrower existing contract. |
| R1-13 | Deferred — transport-gate negative | The production Bun-and-standalone conjunction remains intact, but the missing Bun/non-standalone unit cell was not added. Node transport tests alone must not be called coverage of that cell. |
| R1-14 | Deferred — redirect rejection matrix | Protocol/userinfo/location guards remain, along with tested origin stripping and hop bounds. The additional missing/invalid Location and forbidden-target negatives were not added; no exploitable new credential leak is claimed. |
| R1-15 | Deferred — response-shape matrix | Native dist-tags body/tag checks and current-version fallback remain unchanged. Missing-tag/non-object/non-string native negatives remain test follow-up work. |
| R1-16 | Behavior chosen and partly pinned; diagnostics deferred | Non-ENOENT config reads deliberately fail closed and that policy is now documented and pinned for directory-valued empty paths. Independent npm comparison records the log-and-continue difference as observation. Per-tier errno/path diagnostics and all-errno coverage remain deferred; no raw cause was introduced. |
| R1-17 | Partially covered; exact URL assertion deferred | New auth tests pin parent/bare-host lookup and longest-match precedence; independent controls cover root credentials at a child registry and sibling rejection. The requested combined slashless registry + derived dist-tags URL mutation assertion is not added and remains deferred. |
| R1-18 | Deferred — full error-chain assertion | No raw error cause was added. Existing String(error) checks are unchanged; inspect/cause-chain redaction assertions remain a follow-up and are not claimed fixed by the config behavior repair. |
| R1-19 | Fixed | Key and value interpolation now applies to npmrc and environment sources. Focused cases cover both sources; unchanged independent template-auth-key and template-scope-key comparisons now exactly match real npm 11.17.0. |
| R1-20 | Fixed mismatch; proposed empty skip not adopted | Environment values are trimmed before expansion. Both config-path comparisons and leading-token-space now match the real authority; a unit case keeps whitespace-only credentials from reviving the same lower-tier key. Whitespace-only config paths remain fail closed, recorded as observation rather than inventing fallback parity. |
| R1-21 | Deferred — Windows path behavior | The backslash-tilde form is unchanged and not natively tested. Keep this Windows-specific behavior gap explicit for the supported-platform acceptance batch. |
| R1-22 | Proposed fallback not adopted; contract clarified | Real npm 11.17.0 resolves explicit empty config paths to cwd and records EISDIR before continuing; empty prefix selects cwd/etc/npmrc in the owned differential fixture. The suggested universal || fallback is not supported by those observations. Keep fail-closed read policy and cwd-relative prefix semantics; all three cases remain observations. |
| R1-23 | Fixed | Auth lookup now tests slash-terminated, slashless and bare-host keys in longest-match order without matching sibling prefixes. Focused cases and unchanged real-npm bare-host/slashless comparisons pass. |
| R1-24 | Deferred — runtime sentinel assertion | The runtime installed-manifest assertions retain their prior prefix/interior shape. Exact full-message/sentinel non-leakage strengthening remains paired with R1-6; broad runtime green is not this missing assertion. |
| R1-25 | Maintainer decision / release acceptance | Node 24.19.0 and Bun 1.3.14 remain explicit regression pins, not proof of the actual release-lane Node version. Deriving pins and adding the shipped-version matrix belong with release acceptance; no new release-tool constant is introduced here. |
| R1-26 | Maintainer decision — provisioning trust | The configurable Node mirror still supplies both archive and checksum; the --ci downloader still inherits TLS environment. Local pre-provisioned runtime hashes do not certify that acquisition path. Independent digest trust/TLS override policy and warnings remain open release-infrastructure work. |
| R1-27 | Deferred — not fixed | The committed --ci outer 600-second / per-host 300-second relationship is unchanged. The author runner’s separate 720-second budget is not a production harness fix. Successful local hosts do not prove timeout cleanup/evidence behavior under cumulative overload. |
| Additional P3 | Maintainer decision — compatibility/security policy | Initial HTTP registries may still receive configured authorization, matching npm. HTTPS-only or header suppression would change supported registry behavior; no policy change is made here. Cross-origin redirect stripping remains enforced. |
中文说明
356890b743e142f4226cb86de3bd8c9b29a92baf 的 review 处置
保存的远端快照包含：d41362f 上一轮正式 review，0 Critical / 27 Suggestions；之后同一旧 head 上三份批准；另有一条 HTTP 鉴权 P3。下表已逐项对照当前修复源码。旧批准和 CI 绿灯不能覆盖新提交；新 head 的 CI 进行中，尚未确认通过。Reviewer 自述实验不改称为我们已经动态复现的证据。
本次修复闭合独立复现的 Node 启动器激活缺陷及七项 npm 配置/鉴权差异，不声称全部 Suggestions 已解决。明确延期项继续通过本评论跟踪，策略取舍留给维护者。双语设计同步说明已接受边界和限制。
| 条目 | 处置 | 当前证据／后续范围 |
|---|---|---|
| R1-1 | 维护者决策 | 本版保留 provisioning 失败可见的门禁。网络/工具依赖、准备失败使后续 scripts 不启动以及发布预算耦合均保留披露；移动门禁或接受离线跳过需发布维护者决定。 |
| R1-2 | 明确延期，未修复 | 请求期限仍跨多个等待，本次未改 hosted 测试。此前普通/固定延迟清理结果不能证明 reviewer 所述累计慢宿主时序安全；与 R1-3 一起后续调整预算。 |
| R1-3 | 明确延期，未修复 | 三处默认预算等待未改，不能宣称全部等待均采用宿主感知预算。与 R1-2 成对跟踪，不在本次 registry/launcher 修复中扩大范围。 |
| R1-4 | 单元断言延期；已有真实 runtime 覆盖 | shell stub 仍不执行 launcher payload。现有八个真实解释器坏启动器负控覆盖双 flavor 及直接/预下载激活，另有独立 Node 负控，但不等于已补所要求的 unit mutation/开关覆盖。 |
| R1-5 | 明确延期，未修复 | 保留真实解释器归档复制及行为覆盖；后续在真实 runtime 正反对照下优化 I/O，不用仅 printf 的替身换取速度。 |
| R1-6 | 明确延期，未修复 | 已安装 manifest 的 sentinel 仍未明确断言不能出现在完整错误中；与 R1-24 一并在有界测试加固批次补精确/脱敏断言。 |
| R1-7 | 延期至原生平台验收 | 源码中的 win/win32 映射仍正确，但未新增或执行所要求的 Windows 身份回归；POSIX 通过不算 Windows 通过。 |
| R1-8 | 明确延期：诊断断言 | 仍有安装保留断言，但未新增下载 manifest 非 object 拒绝文案断言；非法 JSON 语法与非 object JSON 应分别约束。 |
| R1-9 | 明确延期：spawn 错误覆盖 | 固定脱敏错误及超时/缓冲选项保留；未新增实际可达 error-event、kill-signal 或 max-buffer 变异测试，单独 EACCES 夹具只会到 callback 分支。 |
| R1-10 | 明确延期：安全诊断 | raw stderr 仍不进入用户错误，也未新增日志；后续需与 R1-9/R1-18 一起决定安全诊断去向并测试脱敏，不直接附加原始 cause/output。 |
| R1-11 | 部分修复；其余路径延期 | Node/Bun 激活前均以 --help 加载生产启动器；原 Node 坏启动器失败已独立闭合，八个真实 runtime 拒绝案例通过。普通命令子进程分支明确不在该 probe 内，继续延期。 |
| R1-12 | 明确延期：诊断一致性 | 未知 runtime 仍按原通用文案 fail closed；未实现路径/恢复指引统一，已有详细恢复指引仍只适用于非法 JSON/非 object 分支。 |
| R1-13 | 明确延期：传输门禁负控 | 生产 Bun 且 standalone 条件仍完整，但未新增 Bun/non-standalone 单元格；不能用 Node 传输测试冒称覆盖该格。 |
| R1-14 | 明确延期：重定向拒绝矩阵 | 协议/userinfo/location 守卫保留，已有 origin 去鉴权及跳数边界测试；未新增缺失/非法 Location 和禁止目标负控，也不将其夸称新的可利用凭据泄漏。 |
| R1-15 | 明确延期：响应形状矩阵 | 原生 dist-tags body/tag 守卫及当前版本 fallback 未变；原生缺失 tag/非 object/非 string 负控继续作为测试跟进。 |
| R1-16 | 行为已明确并部分固定；诊断延期 | 非 ENOENT 配置读取刻意 fail closed，现已文档化并通过空路径解析为目录的测试固定。独立 npm 对照将其与记录后继续的差异保留为观察。逐层 errno/path 诊断及全 errno 覆盖延期，未引入原始 cause。 |
| R1-17 | 部分覆盖；精确 URL 断言延期 | 新增 auth 测试固定父路径/裸 host 查找及最长匹配；独立正控覆盖子 registry 使用根凭据和兄弟路径拒绝。未新增“不带斜杠 registry + 派生 dist-tags URL”的组合变异断言，继续延期。 |
| R1-18 | 明确延期：完整错误链断言 | 未添加原始 error cause。现有 String(error) 断言未改；inspect/cause-chain 脱敏仍需后续补充，不能由配置行为修复推定已解决。 |
| R1-19 | 已修复 | npmrc 与环境来源均展开键和值。定向案例覆盖两种来源；沿用原输入的 template-auth-key、template-scope-key 独立对照现与真实 npm 11.17.0 精确一致。 |
| R1-20 | 已修复差异；不采纳空值跳过建议 | 环境值在展开前修剪；两个配置路径对照及 token 前导空白现与真实 authority 一致，单测固定纯空白凭据不恢复同名低层键。纯空白配置路径继续 fail closed，作为观察记录，不臆造 fallback 等价。 |
| R1-21 | 明确延期：Windows 路径行为 | 反斜杠 tilde 形式未改，也未原生测试；在支持平台验收批次明确跟踪该 Windows 行为缺口。 |
| R1-22 | 未采纳所提 fallback；合同已澄清 | 真实 npm 11.17.0 将显式空配置路径解析为 cwd，记录 EISDIR 后继续；自有差分夹具中空 prefix 选择 cwd/etc/npmrc。因此观察不支持统一改为 |
| R1-23 | 已修复 | 鉴权按最长匹配逐级尝试带尾斜杠、不带斜杠和裸 host 键，不匹配兄弟前缀。定向案例及原真实 npm bare-host/slashless 对照均通过。 |
| R1-24 | 明确延期：runtime sentinel 断言 | runtime 已安装 manifest 仍使用原前缀/内部短语断言；完整错误/sentinel 不泄漏加固与 R1-6 成对跟踪，runtime 整体绿灯不等于新增该断言。 |
| R1-25 | 维护者决策／发布验收 | Node 24.19.0/Bun 1.3.14 保留为明确回归版本钉，不能证明实际发布 lane 的 Node 版本。派生版本钉及增加真实发布版本矩阵归发布验收，本批不新增发布工具常量。 |
| R1-26 | 维护者决策：provisioning 信任 | 可配置 Node 镜像仍同时提供归档和 checksum，--ci 下载器仍继承 TLS 环境。本地预置 runtime 哈希不能证明该下载路径；独立摘要信任、TLS 覆盖策略和告警保留为发布基础设施待办。 |
| R1-27 | 明确延期，未修复 | 已提交 --ci 外层 600 秒/单宿主 300 秒关系未改。作者验证 runner 的独立 720 秒预算不是正式 harness 修复；本地双宿主成功不能证明累计超时下的清理和证据行为。 |
| Additional P3 | 维护者决策：兼容／安全策略 | 初始 HTTP registry 仍可接收已配置鉴权，与 npm 一致。仅 HTTPS 或禁止该 header 会改变支持的 registry 行为，本批不改策略；跨 origin 重定向去鉴权保持。 |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approving at head 356890b743e142f4226cb86de3bd8c9b29a92baf.
My earlier approval of d41362f40f (and two others) was auto-dismissed by this push. That is a stale-head dismissal, not a finding — nothing in it was withdrawn on the merits. This is a fresh review of the new head.
Historical blocking issues — none exist
The only review round is qwen-code-review-bot COMMENTED at the previous head d41362f40f, and its ledger carries 27 findings of which every one is sev:"S". No sev:"C" entry, no CHANGES_REQUESTED and no [Critical] inline comment has ever been filed on this PR. No review round exists at this head yet. Suggestions do not gate this verdict, so I did not spend budget adjudicating them — though the delta turns out to answer four of them, noted below.
The delta from the dismissed head
compare d41362f40f...356890b743 is 1 commit, 7 files, and only two are production code: standalone-update.ts +10/−12 and npm-registry-config.ts +16/−13. The rest are two test files, the runtime acceptance script and the two design docs. I read both production patches in full.
1. The launcher probe is now unconditional rather than Bun-only — verified safe, and a strengthening. This was the one change in the delta that could have been a regression: the previous code probed lib/cli-entry.js --help only when runtime === 'bun', so if a Node standalone archive lacked that file, running the probe for Node would fail the smoke test and abort every Node update. It does ship for both. In scripts/create-standalone-package.js at this head, cli-entry.js is in DIST_REQUIRED_PATHS (so its absence fails packaging outright) and in DIST_ALLOWED_ENTRIES with the comment that it is the bin wrapper the standalone shims use so daemon startup gets the same fast path as npm installs. Both generated launchers invoke it with the runtime as a variable — the unix shim exec "${unixRuntime}" "$ROOT/lib/cli-entry.js" "$@", and the Windows shim resolving windowsRuntime to %ROOT%\bun\bun.exe or %ROOT%\node\node.exe before "%ROOT%\lib\cli-entry.js" %*.
So lib/cli-entry.js is the real entry point of the shipped launcher for both flavors, and the previous Bun-only guard meant a broken Node launcher passed the smoke test and was then committed over a working install. Probing it unconditionally closes that gap. The supporting fixtures moved with it: the unit fixture now writes lib/cli-entry.js unconditionally instead of only for runtime === 'bun' (which also resolves round-1's R1-4, that the fixture file was never loaded by anything), and test-standalone-runtimes.mjs now runs the launcher-state matrix — missing, syntax-error, launcher-error — for every runtime rather than Bun alone.
2. ${VAR} interpolation extracted and applied to keys and the env tier. The inline replacer in readNpmrc became a named expandEnv, now applied to both sides of a file entry (config.set(expandEnv(npmrcValue(match[1])), expandEnv(npmrcValue(match[2])))) and to the environment tier (env.set(expandEnv(name), expandEnv(value.trim()))). The regex and the odd-backslash escape handling are byte-identical to what they replace, so this relocates logic rather than changing it. It answers round-1's R1-19 (npm interpolates config keys, not only values) and R1-20 (the env tier stored process.env values verbatim while both file tiers trimmed). The added .trim() on env values now makes all three tiers agree, matching npm.
I checked this for credential widening, since it reads process.env into config that later becomes an Authorization header. It does not create a new exposure: the registry still comes only from @scope:registry/registry, the credential walk still keys on //host<path>:, the whole path is still gated on isBunRuntime() && isStandalone, and an npmrc that points registry at a hostile host could already exfiltrate a plaintext token written in that same file. Interpolating env vars into npmrc is npm's own documented behaviour, so this narrows the divergence from npm rather than adding a sink.
3. The credential path walk now covers non-trailing-slash keys and still terminates.
if (authPath === '') break;
authPath = authPath.replace(/([^/]+|\/)$/, '');traced from the normalised registry.pathname (which always ends in /): /prefix/ → key //host/prefix/:, then /prefix → key //host/prefix:, then / → key //host/:, then '' → break. The root case / yields exactly //host/: then breaks, identical to before. Each replace removes at least one character, so the loop strictly shortens and cannot spin. This answers round-1's R1-23, that the walk only ever built trailing-slash keys where npm also matches the unslashed form.
Everything verified at the previous head still stands
The delta touches none of it, so I am carrying these forward on the file-set intersection rather than re-deriving them: the manual-redirect loop in updateCheck.ts that strips Authorization monotonically on any cross-origin hop, allowlists https:/http:, rejects embedded userinfo, bounds the chain at six requests, cancels each intermediate body and caps the whole chain with one AbortSignal.timeout; the new Headers({ Authorization }) probe in npm-registry-config.ts that rejects CR/LF in a token, and the two fixed error strings that carry neither token nor URL; the runtime-aware archiveFilename suffix and the manifest read that throws on an unrecognised runtime rather than falling back; and the smoke test's interpreter-identity assertion on self-reported {runtime, platform, arch}, its semver.eq against the expected version, its private mkdtemp probe directory, and the NODE_OPTIONS/BUN_OPTIONS/CLI_VERSION/QWEN_CODE_RELAUNCH_ARGS scrub with killSignal: 'SIGKILL'.
Scope disclosure — what I did not read. I did not audit scripts/test-standalone-runtimes.mjs (+1072) beyond the launcher-matrix hunk above, the four test files (~+1,420), or the design-doc additions. Those are gate and test infrastructure rather than shipped product code, so a defect there surfaces as a false green rather than a user-facing fault. I am asserting no provable Critical in what I read, not the absence of one in what I did not.
CI
No failing checks at this head — 15 pass, 14 skipped, 9 pending. Test (ubuntu-latest, Node 22.x), Lint & Static's companions, the native boundary lanes, Serve A/B, Install (ubuntu-latest), arm64/x64 and review-pr are still running after the 02:19 push. Pending lanes are not a gate and I did not wait on them; nothing reported is attributable to this PR. The unit suites that would exercise the two changed production files are among the pending, so this approval rests on the code reading above rather than on a green test run at this head.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Delta re-review at 356890b7. My earlier approval was pinned to d41362f4 and was dismissed by this push, so I re-read the one-commit delta (7 files) rather than the previously reviewed 3,200-line surface.
What the delta changes, and what I checked at this head:
standalone-update.tsnow runs thelib/cli-entry.js --helplauncher probe for Node as well as Bun, and the Node fixture writes the launcher. Both archive flavors ship that file: it is inDIST_REQUIRED_PATHS, andcreate-standalone-package.jscopies every dist entry intolib/for both flavors, so the probe cannot fail on a valid Node payload. For Node on POSIX,cli-entry.jstakes its in-process fast path for--helpand imports../lib/cli.js, which is the same file the--versionprobe already exercises.npm-registry-config.tsnow expands${...}in config keys as well as values, trims env fields, and replaces the auth-path walk. I tracedhttps://registry.test/private/child/:/private/child/→/private/child→/private/→/private→/→ bare host, then stop. That is both slash-terminated and slashless forms plus the bare host, never a sibling or a path prefix (//registry.test/othstays unmatched), and it closes the oldif (authPath === '/') breakthat never probed the bare host.- Tests at this head: I ran
npm-registry-config.test.tsandstandalone-update.test.tslocally with a scoped config (noglobalSetup; neither file needs workspace dist) — 167 passed, 0 failed. That includes the new slashless/bare-host, templated-key and whitespace-token cells. - Design docs: the EN and ZH twins got the same three additions (key+value expansion and trimming, auth walk order, unreadable config fails closed, launcher probe for both runtimes, and the explicit note that the probe does not cover the launcher's ordinary child-spawn path); both agree with the code.
CI: Lint & Static is green at this head. Test (ubuntu-latest), Serve A/B and the review lane were still running or queued when I submitted, so I am not claiming a green matrix — this verdict rests on the code read and the two test files above.
Non-blocking, unchanged by this delta:
- [P3]
npm-registry-config.ts:141accepts anhttp:registry, so a plaintext-transport config still getsAuthorizationattached. That matches npm; only worth changing if you want to refuse plaintext registry traffic outright. - [P3]
expandEnvleaves${VAR}literal when the variable is unset, so an unset token is sent as the literalBearer ${VAR}and fails on the remote side; npm throws locally instead. Neither leaks anything, but the diagnostic is worse.
No P0/P1. The delta closes the previous round's two real gaps — launcher validation was Bun-only, and credential lookup never reached the bare host — and both are pinned by tests that pass here.
Follow-up disposition: unset environment variable P3For review 5594419746, checked against No new Critical is established by this comment. An earlier, clearer missing-variable diagnostic is explicitly deferred as a non-blocking configuration-policy follow-up; it is not fixed or accepted as a demonstrated npm compatibility regression in this revision. After the completed review rounds, this PR remains limited to Critical fixes. The separately tracked HTTP-registry policy is unchanged. 中文说明已按 该评论未建立新的 Critical。更早、更明确的缺变量诊断作为非阻塞配置策略改进明确延期，本版不称已修复，也不把它定性为已证明的 npm 兼容性回归。按多轮 review 后仅收 Critical 的范围，本 PR 不扩张；已有 HTTP registry 策略处置不变。 |
CI failure reproduced and repaired —
|
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-approved at 127455a0. The delta since 356890b7 is one commit, 127455a0, +18 lines in scripts/test-standalone-runtimes.mjs and nothing else — compare lists exactly that file, so the production code here is byte-identical to what I approved at 356890b7 (5594419746), as is the tree @qqqys approved there; the push dismissed both.
What I checked in the test isolation:
NODE_COMPILE_CACHEnow points at an ownedroot/node-compile-cacheandNODE_DISABLE_COMPILE_CACHEis dropped, which moves the runtime cache the failed gate choked on out ofTMPDIRwhileassert.deepEqual(fs.readdirSync(tmp), [])keeps full strength. Isolating that artifact instead of disabling the cache is the right shape, and the added positive control would go red if caching were ever switched off rather than relocated.- The env mutation stays worker-scoped:
worker()restores fromoriginalEnv, so removing the disable flag does not leak into other cases. node --checkon the changed script passes.
Two non-blocking notes:
- The new positive control proves the compile cache is enabled and writing, not that the payload probe produced those entries — the worker's own dynamic imports satisfy it too. I reproduced that locally: with
NODE_COMPILE_CACHEset, a singleimport()wrote 3 cache files on Node 24.19.0. Good enough as a "caching is not silently off" witness; just note the assertion message claims more than the check isolates. - Housekeeping on the consolidated stack: #14139/#14142/#14210 are closed, but #14134 and #14187 are still open while their content lives here, and the linkage is
Related to #11607, so nothing closes automatically. Worth closing both once this lands.
CI at this head is still mid-flight (8 lanes) and the branch is 21 commits behind main; neither is a code finding.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approving at head 127455a069bad15591262b1326c4e7eed8a0e6f9.
My approvals of d41362f40f and 356890b743 were both auto-dismissed by subsequent pushes. Those are stale-head dismissals, not findings — nothing in either was withdrawn on the merits. This is a fresh review of the new head.
Historical blocking issues — none exist
The only review round remains qwen-code-review-bot COMMENTED at d41362f40f, whose ledger carries 27 findings of which every one is sev:"S". No sev:"C" entry, no CHANGES_REQUESTED and no [Critical] inline comment has ever been filed on this PR, and no review round exists at this head. Suggestions do not gate this verdict.
The delta from the head I last reviewed is test infrastructure only
compare 356890b743...127455a069 is 1 commit, 1 file: scripts/test-standalone-runtimes.mjs +18/−0. I cross-checked this against the PR's own file list at the new head rather than trusting the compare alone, and all four production surfaces are unchanged — standalone-update.ts +220/−47, updateCheck.ts +63/−7, npm-registry-config.ts +176, package.json +3/−1, identical counts to the previous head. So the production verification below carries forward on a zero-intersection blast radius, not on assumption.
The 18 new lines do three things, all in the acceptance harness:
- Give the worker a private
NODE_COMPILE_CACHEunder its own root, so Node's compile cache is sandboxed alongside the existingHOME/TMPDIR/TMP/TEMPisolation instead of landing in the host's real cache. delete process.env.NODE_DISABLE_COMPILE_CACHE, so an inherited disable flag cannot silently turn the cache off and make the isolation vacuous.- Add a positive control: for the
node/direct/version-1.2.3case, assert that the compile-cache directory actually contains at least one file.
That third addition is the one worth noting, because it is the correct shape — it asserts the isolation took effect rather than assuming it, and it sits alongside the pre-existing leak checks (process.env restored to parentEnv, tmp empty, install directory unchanged). No assertion in the delta is weakened, removed or broadened; one is added.
Two residuals I judged non-blocking and am not filing. The new fs.readdirSync(compileCache, { recursive: true, withFileTypes: true }) will throw ENOENT rather than report the assertion message if Node never created the directory at all — a less legible failure, but it still fails, so it cannot produce a false green. And the added NODE_COMPILE_CACHE key participates in the surrounding assert.deepEqual({ ...process.env }, parentEnv) restoration check; I found no evidence that snapshot is taken before the Object.assign, and asserting a break there would be speculation rather than proof.
Production verification carried forward
Nothing in the delta touches these, and I verified each at the previous head:
updateCheck.ts— the native dist-tags path is gated onisBunRuntime() && isStandalone, usesredirect: 'manual', and stripsAuthorizationmonotonically on any cross-origin hop (authis only ever reassigned toundefined, never restored, so a chain that leaves and returns cannot re-attach the token). The origin comparison includes protocol, so anhttps:→http:downgrade also drops it. Protocol is allowlisted tohttps:/http:, embedded userinfo is rejected, the loop is bounded at six requests, each intermediate body is cancelled, and oneAbortSignal.timeoutcaps the whole chain. Response validation refuses a non-object/array body and a non-string tag, and an absent tag yieldslatest ?? currentVersion.npm-registry-config.ts— registry resolution follows npm precedence (@scope:registry→registry→ public default), the merged tier order puts env last, and the project.npmrcstays excluded to match the previousnpm view --globalbehaviour. The URL is normalised before credential keys are built.expandEnvis applied to both keys and values across the file and env tiers, matching npm. Both failure paths throw fixed strings carrying neither token nor URL, and thenew Headers({ Authorization })probe rejects CR/LF in a token. The credential walk covers trailing- and non-trailing-slash keys and terminates because eachreplacestrictly shortens the path.standalone-update.ts—archiveFilenameis runtime-aware;standaloneUpdateTargetthrows on an unrecognised manifestruntimerather than falling back, and wraps the manifest parse in a recovery instruction that tells the user to keep the installation and backup. The smoke test asserts the shipped interpreter's self-reported{runtime, platform, arch}against the parsed target, requiressemver.eq(version, expectedVersion), probes in a privatemkdtempdirectory so nobunfig.tomlpreload is inherited, scrubsNODE_OPTIONS/BUN_OPTIONS/CLI_VERSION/QWEN_CODE_RELAUNCH_ARGS, useskillSignal: 'SIGKILL', and removes its temp directory in afinally. The launcher probe overlib/cli-entry.js --helpis unconditional, which I confirmed is correct:cli-entry.jsis inDIST_REQUIRED_PATHSandDIST_ALLOWED_ENTRIESinscripts/create-standalone-package.js, and both generated shims exec it with the runtime as a variable — so the previous Bun-only guard let a broken Node launcher pass.
Scope disclosure — what I did not read. I did not audit the full 1,090-line scripts/test-standalone-runtimes.mjs beyond the delta above, the four test files (~+1,520), or the two design-doc additions. Those are gate and test infrastructure rather than shipped product code, so a defect there surfaces as a false green rather than a user-facing fault. I am asserting no provable Critical in what I read, not the absence of one in what I did not.
CI
No failing checks at this head — 13 pass, 9 skipped, 8 pending after the 03:21 push, including Test (ubuntu-latest, Node 22.x) (which runs the scripts suite that chains test:standalone:runtimes:ci), Lint & Static, Integration Tests (no-AK, No Sandbox), the two native boundary lanes, Install (ubuntu-latest), Serve A/B and review-pr. Pending lanes are not a gate and I did not wait on them. Worth stating plainly: because that test lane is still running, the newly added compile-cache assertion has not been confirmed green at this head — this approval rests on the code reading above, and the lane is where a harness mistake would surface.
|
Follow-up on the two non-blocking notes in the current-head review:
Current |
|
Runtime acceptance update for The previous head's Ubuntu job exceeded the unchanged 300-second Node-worker limit. The minimal repair only changes the archive fixture to explicit fast gzip compression (+14/-10 in one test script). No production behavior, case, assertion, version pin or timeout changed. Fresh build/typecheck/lint/format passed. Independent execution of the complete official matrix passed on macOS arm64: Node 24.19.0 147.529s, Bun 1.3.14 152.010s, both exit 0; TLS and empty-temp assertions remained enabled. The unchanged original Node fixture had passed locally in 217.413s; this is a single-host comparison, not proof of a Linux fix. All 344 observed process identities had exited, owned process group was empty and scratch was removed; no cleanup signals were required. The two worker logs contain the same event-shape counts as the previous complete matrix. Bun raw child-probe observation remains unavailable, not zero operations. The earlier Web Shell smoke failure is separate: the case stopped before permission assertions because the app failed to load. Its saved retry trace has 25 Fresh CI is running. Local success does not replace that gate. Independent receipt SHA256: 中文：本次只在原 PR 内优化测试归档压缩，未改生产行为、用例、断言、版本或超时。独立完整双宿主矩阵及构建/类型/静态检查通过，Node/Bun 分别约148/152秒，所有已观察进程与 scratch 均清理。本机通过不等于 Linux 已修；新 CI 正在运行。旧 Web Shell 失败发生在权限断言前的资源加载，trace 有25个 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM at 65c20e3f. The only change since the previously approved 127455a0 is one test-only commit — test(cli): reduce standalone runtime fixture compression cost (+14/−10 in scripts/test-standalone-runtimes.mjs). No production file, case, assertion, version pin or timeout changed; the production tree is byte-identical to 127455a0, so the previous review still covers it.
What I verified on the delta:
packArchive()keeps the same output path:archive.slice(0, -3)turns...tar.gzinto...tar, andgzip -1 -frecreates...tar.gzbeforesumshashes it. Ran both commands by hand on a synthetic payload —gzip -tpasses andtar -tzfyields the sameqwen-code/…entry list.ci()still returns early onwin32, so the newgzipdependency only applies to the ubuntu/macOS lanes that already ship it.- The intermediate
.tarlands inroot/, while the emptiness assertions readroot/tmp(line 74), so the fixture's tmp assertions keep their strength;gzipalso removes its input. - Both call sites (normal pack, and the
missing/launcher-errorrepack) now share the helper, so the previous per-state archive refresh behaviour is unchanged.
Carried over from the earlier round, non-blocking and unchanged: the compile-cache positive control proves caching is on and writing, not that the probe itself wrote it; and #14134/#14187 stay open although their content is in this PR (the body only says Related to #11607).
CI at this head is still in flight (Lint & Static, Test, Integration, Install, native lanes). Reported as status, not as a finding.
中文
65c20e3f 通过。相对我上一轮批过的 127455a0，增量只有一笔测试改动（scripts/test-standalone-runtimes.mjs +14/−10，把 tar -czf 拆成 tar -cf + gzip -1 -f 降低 fixture 压缩开销），生产代码字节未变，上一轮的核验继续有效。
核过：archive.slice(0,-3) + gzip -1 -f 仍产出同名 .tar.gz，sums 在压缩后计算；本机实测两条命令 gzip -t 通过、tar -tzf 条目一致。ci() 在 win32 提前返回，gzip 只落在 ubuntu/macOS 车道。中间 .tar 落在 root/，空目录断言读的是 root/tmp，强度不变。两处打包调用（正常打包与 missing/launcher-error 重打包）行为一致。
沿用上一轮的非阻塞项：编译缓存正控只证明「缓存开着且在写」；#14134/#14187 仍 open 而内容已在本 PR。当前 head 的 CI 仍在跑，仅作状态记录，不算 finding。
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approving at head 65c20e3fcd4360222655b6e7af0e2dbbe67e86cb.
My approvals of d41362f40f, 356890b743 and 127455a069 were each auto-dismissed by the next push, as were the maintainer's. Those are stale-head dismissals, not findings — nothing in any of them was withdrawn on the merits. This is a fresh review of the new head, and the fourth in about three hours; worth noting only because each push invalidates the approvals again, so landing this before the next one would avoid a fifth round.
Historical blocking issues — none exist
The only review round remains qwen-code-review-bot COMMENTED at d41362f40f, whose ledger carries 27 findings of which every one is sev:"S". No sev:"C" entry, no CHANGES_REQUESTED and no [Critical] inline comment has ever been filed on this PR, and no review round exists at this head. Suggestions do not gate this verdict.
The delta is a test-harness speed change with no production surface
compare 127455a069...65c20e3fcd is 1 commit, 1 file: scripts/test-standalone-runtimes.mjs +14/−10. I confirmed this against the PR's own file list at the new head rather than the compare alone, and all four production surfaces are unchanged — standalone-update.ts +220/−47, updateCheck.ts +63/−7, npm-registry-config.ts +176, package.json +3/−1, identical counts to the head I reviewed two rounds ago. The production verification therefore carries forward on a zero-intersection blast radius, not on assumption.
The 14 lines extract a packArchive() helper and switch fixture compression from tar -czf (gzip default level) to tar -cf followed by gzip -1 -f. I checked the three ways this could have been wrong:
- The artifact is still the file the production path expects. The output name is unchanged (
archive), the format is still gzip, andgzipconsumes its input so no stray.taris left in the fixture root to trip the harness's directory-emptiness assertions. Compression level is invisible to extraction, so the code under test sees an identical shape. - The checksum still describes the real artifact.
sums = ${sha(archive)} ${filename}is computed after thegzipstep, not after thetar, soSHASUMS256.txtmatches the gzipped bytes the update actually verifies. Reversing that order would have made every checksum gate pass vacuously or fail outright. - The intermediate name is derived correctly.
archive.slice(0, -3)strips the.gzfrom a filename that always ends.tar.gz, andgzip -1 -frestores exactlyarchive; the-fis what allows the same path to be repacked across the state matrix.
Both former inline tar -czf sites now call the one helper, so the initial pack and the per-state repack cannot drift apart — previously they were two copies of the same five arguments. No assertion in the delta is weakened, removed or broadened.
Production verification carried forward
Nothing in the delta touches these, and I verified each at 356890b743:
updateCheck.ts— the native dist-tags path is gated onisBunRuntime() && isStandalone, usesredirect: 'manual', and stripsAuthorizationmonotonically on any cross-origin hop (authis only ever reassigned toundefined, never restored, so a chain that leaves and returns cannot re-attach the token). The origin comparison includes protocol, so anhttps:→http:downgrade also drops it. Protocol is allowlisted tohttps:/http:, embedded userinfo is rejected, the loop is bounded at six requests, each intermediate body is cancelled, and oneAbortSignal.timeoutcaps the whole chain. Response validation refuses a non-object/array body and a non-string tag, and an absent tag yieldslatest ?? currentVersion.npm-registry-config.ts— registry resolution follows npm precedence (@scope:registry→registry→ public default), the merged tier order puts env last, and the project.npmrcstays excluded to match the previousnpm view --globalbehaviour. The URL is normalised before credential keys are built,expandEnvapplies to keys and values across the file and env tiers, both failure paths throw fixed strings carrying neither token nor URL, and thenew Headers({ Authorization })probe rejects CR/LF in a token. The credential walk covers trailing- and non-trailing-slash keys and terminates because eachreplacestrictly shortens the path.standalone-update.ts—archiveFilenameis runtime-aware;standaloneUpdateTargetthrows on an unrecognised manifestruntimerather than falling back, and wraps the manifest parse in a recovery instruction that tells the user to keep the installation and backup. The smoke test asserts the interpreter's self-reported{runtime, platform, arch}against the parsed target, requiressemver.eq(version, expectedVersion), probes in a privatemkdtempdirectory so nobunfig.tomlpreload is inherited, scrubsNODE_OPTIONS/BUN_OPTIONS/CLI_VERSION/QWEN_CODE_RELAUNCH_ARGS, useskillSignal: 'SIGKILL', and removes its temp directory in afinally. The launcher probe overlib/cli-entry.js --helpis unconditional, which is correct:cli-entry.jsis inDIST_REQUIRED_PATHSandDIST_ALLOWED_ENTRIESinscripts/create-standalone-package.js, and both generated shims exec it with the runtime as a variable.- The harness isolation added at the previous head also stands: a private
NODE_COMPILE_CACHEunder the worker root,NODE_DISABLE_COMPILE_CACHEdeleted so the isolation cannot be silently vacuous, and a positive control asserting cache files were actually created.
Scope disclosure — what I did not read. I did not audit the full 1,086-line +1,520), or the design-doc additions. Those are gate and test infrastructure rather than shipped product code, so a defect there surfaces as a false green rather than a user-facing fault. I am asserting no provable Critical in what I read, not the absence of one in what I did not.scripts/test-standalone-runtimes.mjs beyond the deltas at this and the previous head, the four test files (
CI
No failing checks at this head — 8 pass, 9 skipped, 13 pending after the 04:58 push, including Test (ubuntu-latest, Node 22.x) (which runs the scripts suite that chains test:standalone:runtimes:ci, i.e. the harness this commit edits), Lint & Static, Integration Tests (no-AK, No Sandbox), TUI parity snapshots, the native boundary lanes, the three Install lanes, Serve A/B and arm64/x64. Pending lanes are not a gate and I did not wait on them. Stated plainly: because the scripts suite has not reported at this head, the repacked fixture has not yet been confirmed to satisfy the harness's own checksum and leak assertions — this approval rests on the code reading above, and that lane is where a packing mistake would surface.
|
Review consolidation update: the remaining interpreter consumers have now been published in stacked Draft #14959 at 中文：剩余解释器调用点已发布至 Draft #14959 |
|
Post-merge CI evidence for
Both retain the original 300s worker limit. The runtime script ran once without retries or skipped scenarios. Scripts Vitest then completed with 3,316 passes and 13 existing skips; the surrounding Vitest invocation still uses The earlier compile-cache failure was a fixture isolation defect. The later Bun raw-probe/allocation interception remains unavailable. The fixture uses a checksum-only archive with signature 404; successful TLS traffic and a literal TLS flag do not substitute for an untrusted-CA negative test. The separate Web Shell job subsequently completed at exact 中文：当前提交的 Ubuntu 日志确认 Node 24.19.0 与 Bun 1.3.14 各完成 76 个真实 runtime 场景，分别耗时 150.842 秒、154.279 秒，均退出 0、无终止信号、严格临时目录为空。保留原有每 worker 300 秒上限，runtime 脚本没有重试或跳过；之后 scripts Vitest 为 3,316 通过、13 个既有跳过，外围仍配置 此前缓存失败属于测试隔离问题； |
|
Released in v0.26.0. |
What this PR does
Standalone updates preserve the installed Node or Bun runtime flavor through download, validation, activation and rollback. Node retains its npm transport and inherited registry, proxy, CA and authentication configuration. Bun standalone checks release tags without requiring external Node/npm, using the supported builtin, global, user and environment registry/authentication settings while excluding implicit project configuration.
Before activation, both runtime flavors verify the downloaded package identity, interpreter identity, actual CLI version and ability of the production launcher to load the CLI. Prepared updates recheck the installation identity; rejected updates preserve the current installation and rollback backup. Registry lookup now expands environment variables in keys as well as values, trims environment fields, and supports longest-match credentials at slash-terminated paths, slashless paths and the bare host. Unreadable configuration deliberately fails closed instead of silently selecting another registry.
This is the consolidated standalone-update boundary from #14134, #14139, #14142, #14187 and #14210, including the earlier CI cancellation-test cleanup: 11 files, +3222/-133. It does not change the TUI or complete the separate interpreter call-site migration.
Why it's needed
The existing updater selected classic Node archives without preserving a Bun installation's runtime. A launcher's metadata-only version response also does not prove that its CLI can load. Independent reproduction additionally showed that a corrupt Node launcher could be activated, and that templated npmrc keys, whitespace-bearing configuration paths and valid slashless authentication keys selected the wrong registry or credentials. This update rejects the broken launcher before replacement and closes those seven observed npm configuration mismatches.
Reviewer Test Plan
How to verify
Evidence (Before & After)
At
d41362f, independent reproduction accepted a Node archive with a syntactically broken launcher and found seven npm registry/authentication mismatches. Those original failures remain preserved. At356890b, the repaired production source passed build/typecheck/bundle, 215 focused unit tests, two independent launcher cases, six npm controls and all seven failing comparisons. Three empty-value cases remain observations, not passes: empty/whitespace config paths fail closed while npm records EISDIR and continues; an empty prefix retains cwd-relative semantics.The Ubuntu runtime gate at
356890bthen failed because the real Node launcher's help path created a compiler cache in the fixture's temporary directory. The launcher is byte-identical to main. The earlier local runtime run supplied a separate cache directory outside the official script; CI did not. Independent controls reproduced the default help-path residue and showed the same help command passing with an owned cache directory while caching remained enabled. The original CI failure and failed control are preserved.At
127455a, the cache isolation passed locally, but the next Ubuntu runtime gate exceeded its unchanged 300-second Node-worker limit. That worker did not produce a complete receipt and the Bun worker did not start. Independent observation of the unchanged script on macOS passed in 217.413 seconds and confirmed substantial time spent repeatedly compressing real runtime archives; it does not reproduce or explain every part of the Linux timeout.Current head
65c20e3fcd4360222655b6e7af0e2dbbe67e86cbmakes a one-file test-only change (+14/-10): it creates the same real tar archives and explicitly uses gzip level 1 at both packing sites. Every scenario, assertion, runtime version and timeout remains unchanged. Fresh build, typecheck, lint and formatting passed. An independent run of the complete official Node+Bun matrix, with no outer cache override, passed: Node 147.529 seconds and Bun 152.010 seconds; both hosts exited 0, retained TLS verification and strict empty-temporary-directory checks, and exercised both archive flavors including the broken-launcher rejection controls. All 344 observed process identities were gone and owned scratch was removed without cleanup signals. The local improvement is evidence for this fixture optimization, not proof that the Linux timeout is resolved.The same old workflow also had a separate Web Shell browser failure: 337 passed, four passed after retry, one failed. The permission test failed before its permission assertions because the application could not load; the saved retry trace contains 25 local module/CSS requests with
net::ERR_NETWORK_CHANGED. The Web Shell tree, TypeScript SDK tree, lockfile and workflow are identical to the preceding passing run and the PR base. R1 causation is not established; this failure remains recorded and is not claimed fixed by archive compression.The 215 unit cases, independent npm comparisons and bundle evidence above remain attributed to
356890b; those production and test sources are unchanged by the subsequent fixture-only changes. New-head CI and current-head review remain pending; the previous two approvals apply to127455a. The review disposition and unset-variable follow-up retain deferred Suggestions and policy decisions rather than claiming they were all fixed.Tested on
Environment (optional)
Isolated managed worktree; Node 24.19.0, Bun 1.3.14 and real npm 11.17.0 configuration/authentication modules. Fixtures used owned homes, system/default settings, npm configuration, working directories, temporary roots, a local HTTPS server and synthetic archives/tokens. The independent npm comparison made no external registry requests. The Node update-query transport fixture delegates to an injected npm executor; it is not corporate proxy/CA or live npm network acceptance.
Risk & Scope
The English design and Chinese design contain the synchronized runtime boundary, failure semantics, acceptance criteria and remaining limitations.
Linked Issues
Related to #11607; runtime migration context #14046. Integrated source stack: #14134, #14139, #14142, #14187 and #14210. No automatic closure of the overall migration issue.
中文说明
本 PR 的改动
Standalone 更新在下载、验证、激活和回滚过程中保留已安装的 Node 或 Bun runtime 类型。Node 保留 npm 传输以及继承的 registry、代理、CA 和鉴权配置。Bun standalone 查询发布标签不依赖外部 Node/npm，使用受支持的 builtin、global、user 和环境 registry/鉴权配置，同时排除隐式项目配置。
激活前，两种 runtime 都验证下载包身份、解释器身份、实际 CLI 版本以及生产启动器加载 CLI 的能力。预下载更新重新核对安装身份；拒绝更新时保留当前安装和回滚备份。Registry 查询现在同时展开键和值中的环境变量，修剪环境字段，并按最长匹配支持带尾斜杠路径、不带尾斜杠路径和裸 host 的凭据。读取非缺失配置失败时明确拒绝，而不静默选择另一个 registry。
这是整合 #14134、#14139、#14142、#14187、#14210 的完整 standalone 更新边界，包含先前 CI 取消测试的清理修正：11 个文件，+3222/-133。本批不修改 TUI，也不代表独立解释器调用点迁移已完成。
为什么需要
原更新器选择传统 Node 归档，无法保留 Bun 安装的 runtime。启动器只返回元数据版本，也不能证明 CLI 可以加载。独立复现进一步发现：损坏的 Node 启动器可能被激活；带环境模板的 npmrc 键、包含空白的配置路径和有效的不带尾斜杠鉴权键会选错 registry 或凭据。本次更新在替换前拒绝损坏启动器，并闭合实际观察到的七项 npm 配置差异。
审阅者测试计划
如何验证
证据（修改前后）
在
d41362f上，独立复现确认含语法损坏 Node 启动器的归档被接受，并发现七项 npm registry/鉴权差异；原始失败均保留。在356890b上，修复后的生产源码通过 build/typecheck/bundle、215 个定向单元测试、两个独立 launcher 案例、六个 npm 正控及原七项差异对照。三个空值案例仅记录观察，不计为通过：空/纯空白配置路径 fail closed，而 npm 记录 EISDIR 后继续；空 prefix 保留相对 cwd 的语义。随后，
356890b的 Ubuntu runtime 门禁失败：真实 Node 启动器的 help 路径在夹具临时目录中生成编译缓存。该启动器与 main 字节一致。此前本地运行在正式脚本外额外提供了独立缓存目录，CI 没有这层配置。独立正反控复现了默认 help 路径的残留，并确认同一 help 命令在自有缓存目录下通过，缓存仍实际启用。原 CI 失败和失败对照均保留。在
127455a上，缓存隔离通过本地验证，但随后 Ubuntu runtime 门禁超过未变的 Node worker 300 秒时限。该宿主未生成完整收据，Bun 宿主尚未启动。独立观察原样脚本在 macOS 上以217.413秒通过，并确认反复压缩真实 runtime 归档消耗显著时间；这不等于复现或完整解释了 Linux 超时。当前 head
65c20e3fcd4360222655b6e7af0e2dbbe67e86cb仅调整一个测试文件（+14/-10）：仍生成真实 tar 归档，两处打包显式使用 gzip level 1。全部场景、断言、runtime 版本和时限保留。新执行的 build、typecheck、lint、格式检查通过；独立运行完整正式 Node+Bun 矩阵也通过，外层未设置缓存覆盖。Node 用时147.529秒、Bun152.010秒，均退出0，TLS验证和严格临时目录为空断言保留，均覆盖两种归档及损坏启动器拒绝控制。观察到的344个进程身份全部消失，自有 scratch 已清理，未使用清理信号。本机耗时改善支持这项夹具优化，但尚不能证明 Linux 超时已解决。同一旧 workflow 还有独立的 Web Shell 浏览器失败：337通过、4项重试后通过、1失败。permission 案例在权限断言前因应用资源加载失败而超时；保存的重试 trace 中有25个本地模块/CSS请求报
net::ERR_NETWORK_CHANGED。Web Shell、TypeScript SDK 整树、lockfile 和 workflow 与前一通过运行及 PR base 相同。尚未建立由 R1 引起的因果关系；保留这项失败，不声称归档优化修好了它。上述215个单元案例、独立 npm 对照和 bundle 证据仍归属于
356890b；后续仅修改测试夹具，相关生产和单元测试源码未变。新 head CI和当前 head 审查仍待完成，前两份批准归属于127455a。review 处置及缺失变量补充明确保留延期 Suggestions 和策略决策，不声称已全部修复。已测试平台
环境（可选）
隔离 managed worktree；Node 24.19.0、Bun 1.3.14 和真实 npm 11.17.0 配置/鉴权模块。夹具使用自有 HOME、system/default settings、npm 配置、cwd、临时根、本地 HTTPS 服务以及合成归档/token。独立 npm 对照不访问外部 registry。Node 更新查询的传输夹具委托注入的 npm 执行器，不代表企业代理/CA 或实际 npm 网络验收。
风险与范围
英文设计和中文设计同步包含 runtime 边界、失败语义、验收标准及剩余限制。
关联 Issue
关联 #11607；runtime 迁移背景 #14046。整合来源 #14134、#14139、#14142、#14187、#14210。不自动关闭整体迁移 issue。