refactor(review): extract the toolchain adapter boundary - #8776
Conversation
`review build-test` combined three responsibilities in one module: reading the review plan, deciding which toolchain can be verified deterministically, and implementing npm workspace installation, affected-package selection, dependency widening, build execution, test execution, and reporting. Move the npm implementation behind an internal `ReviewToolchainAdapter` contract. `build-test.ts` keeps CLI routing, plan reading, output trimming, env shaping, and the spawn boundary; `lib/npm-toolchain.ts` owns npm detection and the verification algorithm; `lib/disk.ts` holds the shared free-disk floors. This is a move, not a rewrite: 95% of the lines removed from build-test.ts reappear verbatim in the new files. The CLI arguments, the BuildTestReport JSON shape, and every npm behaviour are unchanged, and the existing build-test suite is the compatibility oracle for that. Selection requires exactly one applicable adapter and fails closed to the `unsupported` handoff otherwise, so a second toolchain lands as a registration rather than another branch in this file.
|
Thanks for the PR! Template looks good ✓ Problem: this is a structural refactor, not a bug fix, so no reproduction applies. The motivation is concrete: Direction: aligned. This is internal tooling with no user-visible surface, and it directly serves the Maven multi-module verification that #8394 is being split into. The design doc is committed under Size: authored by a maintainer and confined to Approach: the scope fits the stated goal, with one honest question kept from the first pass: Risk: no elevated risk signals — none of the revert-correlated high-risk paths are touched. Re-run note: re-gated at the merged head — two main-sync merges landed since the first pass, with no changes to the PR's own files. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是结构性重构而非 bug 修复，不涉及复现。动机具体： 方向：对齐。这是内部工具改动，无用户可见面，直接服务于 #8394 拆分后的 Maven 多模块验证。设计文档已按约定提交在 规模：作者为维护者，改动限于 方案：就目标而言范围合理，保留第一轮提出的一个值得思考的问题： 风险：无升级风险信号——未触及与 revert 相关的高风险路径。 Re-run 说明：本次在合入 main 后的最新 head 上重新执行门禁——首轮之后有两次 main 同步合并，PR 自身文件无变化。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
Code review — re-read the full diff against the merged head (the two main-sync merges carry no changes to this PR's files) and against an independent baseline (extract the npm algorithm into one module, defer any adapter contract until a second toolchain actually exists). The PR goes one step further than that baseline, and given #8394 is being split into reviewable pieces, it's the better call: the Maven follow-up lands as a registration instead of re-opening No blockers found. The move checks out:
One thing the description understates (flagged on the first pass, standing): this is not 100% a move. Small, intentional, commented-and-tested fixes ride along:
None of this changes happy-path report shapes, and every fix has a dedicated test — but "every npm behaviour is unchanged" is not strictly true, and a reviewer trusting the pure-move framing would read the diff more lightly than it deserves. Not blocking; the thread now records it. Standing non-blocking follow-ups, already surfaced by the Test evidence — the PR's own CI at the reviewed commit, now fully settled (triage policy: never execute PR code locally):
Nothing red and nothing pending on this SHA. The ubuntu leg carries the changed suite; the macOS/Windows unit legs and the integration leg were skipped by CI configuration, consistently with earlier runs on this PR — not verified: those legs. The changed logic is platform-independent budget arithmetic, selection gates, and note text (the pre-existing POSIX/cmd.exe escaping caveat in 中文说明代码审查——在合入 main 后的最新 head 上重新通读了完整 diff（两次 main 同步合并未改动本 PR 的文件），并对照独立基线（我的基线是：把 npm 算法抽到一个模块，等第二个 toolchain 真正出现再定义 adapter 契约）。PR 比这个基线多走了一步，而鉴于 #8394 正在拆成可 review 的小块，这一步是对的：Maven 后续只需注册，不必重新打开 未发现阻塞问题，搬运核对无误：
有一点描述轻描淡写了（首轮已指出，仍然成立）：这并非 100% 的搬运，夹带了几处有意图、有注释、有测试的小修复：
这些都不改变正常路径的报告结构，且每个修复都有专门测试——但"所有 npm 行为未变"严格来说不成立，一个相信"纯搬运"说法的 reviewer 会用偏轻的眼光读这个 diff。不构成阻塞；本线程已记录在案。 遗留的非阻塞跟进项（ 测试证据——PR 自身 CI 在被审 commit 上的结果，现已全部结束（triage 策略：不在本地执行 PR 代码）：上方表格。该 SHA 上无红色、无 pending。ubuntu 腿承载了改动涉及的套件；macOS/Windows 单测腿与集成腿被 CI 配置跳过，与本 PR 此前各轮一致——未验证：这些腿。改动逻辑是与平台无关的预算运算、选择门禁与文案（ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: reverse audit — stopped before round 5 by the review time budget. Test Plan (not a blocker): lib/npm-toolchain.ts — no such file or directory; lib/disk.ts — no such file or directory; lib/npm-toolchain.test.ts — no such file or directory; 2293 tests passed — this review observed 18620 passed.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查：反向审计——评审时间预算不足，未能开始第 5 轮。 Test Plan（非阻断）：lib/npm-toolchain.ts — no such file or directory; lib/disk.ts — no such file or directory; lib/npm-toolchain.test.ts — no such file or directory; 2293 tests passed — this review observed 18620 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| function testCommand(dir: string): string { | ||
| return dir === '.' ? 'npm test' : `npm test --workspace=${shellArg(dir)}`; | ||
| } | ||
| export const toolchainAdapters: readonly ReviewToolchainAdapter[] = [ |
There was a problem hiding this comment.
[Suggestion] R1-20: The adapter registry this PR adds is observably inert to the suite: emptying toolchainAdapters ships the entire suite green (mutation-probed: = [] → 81/81 pass), because every root with a package.json is caught by the hardcoded fallback delegation (the identical call) and every root without one emits the identical generic report; the selectToolchainAdapter tests pass explicit arrays, never the registry. A probe also shows runBuildTest does consult the registry today — it is live but untested, not dead code. — Failure scenario: a follow-up that empties the registry, routes selection through a different list, or registers a second adapter the command never consults ships green → defeats the JSDoc's own promise that "the next one is a registration rather than another branch in this file", and the adapter author has no failing test to tell them selection is being bypassed.
Suggested fix: add one selection-level test proving the command consults the registry — e.g. temporarily push a stub adapter (applies: () => true) into toolchainAdapters, call runBuildTest on an npm-shaped root, expect the ambiguity handoff (which also pins the branch at lines 345-347), then pop it; or at minimum assert the registry's membership so emptying it turns a test red.
中文说明
本 PR 新增的 adapter 注册表对整个测试套件是「观测上无效」的：把 toolchainAdapters 清空后整个套件仍全绿（突变探针验证：= [] → 81/81 通过）。因为任何带 package.json 的根都会被硬编码的兜底委托接住（调用完全相同），不带 package.json 的根无论是否查询注册表都输出相同的通用报告；而 selectToolchainAdapter 的测试传入的是显式数组，从不使用这个注册表。探针同时证明 runBuildTest 目前确实会查询注册表——它是活的但未被测试，不是死代码。— 失败场景：后续改动清空注册表、改由别的列表做选择、或注册了第二个 adapter 而命令实际并不查询，都会全绿通过 → 违背 JSDoc 自己的承诺「下一个 toolchain 是一次注册而不是本文件里的又一个分支」，adapter 作者也没有任何失败测试能提醒选择正被绕过。
修复建议： 加一个选择层测试证明命令确实查询注册表——例如临时向 toolchainAdapters push 一个桩 adapter（applies: () => true），在 npm 形态的根上调用 runBuildTest，断言得到歧义交接（这同时也钉住了 345-347 行的分支），然后 pop 掉；至少也应断言注册表的成员，使清空它时会有测试变红。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| /** | ||
| * The environment every build/test/install command runs under. |
There was a problem hiding this comment.
[Suggestion] R1-5: Test-efficacy probe: reverting the deletion of the disk-preflight constants/helpers (INSTALL_MIN_FREE_BYTES, BUILD_MIN_FREE_BYTES, freeDiskBytes, gib) from this file left every test green — the expected shape of a pure move, but it also means the moved module lib/disk.ts has no direct unit test of its own; its behaviour is only exercised through the public entry. — Failure scenario: a regression inside the moved helpers that the integration-style tests don't reach (e.g. freeDiskBytes error handling, gib formatting) has no direct suite to turn red; stale duplicates re-introduced here would also ship green.
Suggested fix: add a small direct test for lib/disk.ts (freeDiskBytes returning null when statfsSync throws, gib formatting) so the moved module is gated at its own boundary.
中文说明
测试有效性探针：把磁盘预检常量/工具（INSTALL_MIN_FREE_BYTES、BUILD_MIN_FREE_BYTES、freeDiskBytes、gib）从本文件的删除恢复后，所有测试仍全绿——这是纯搬运的预期形态，但同时也意味着搬过去的 lib/disk.ts 没有自己的直接单元测试，其行为只通过公共入口被检验。— 失败场景：搬运后工具内部的回归若落在集成式测试覆盖不到的地方（例如 freeDiskBytes 的错误处理、gib 的格式化），就没有任何直接套件会变红；此处若重新引入过时的重复副本也会全绿通过。
修复建议： 为 lib/disk.ts 补一个小型直接测试（statfsSync 抛错时 freeDiskBytes 返回 null、gib 的格式化），让搬过去的模块在自己的边界上被守卫。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // at the one boundary every command crosses. | ||
| const deadlineMs = Math.max(1, Math.round(timeoutMs)); |
There was a problem hiding this comment.
[Suggestion] R1-9: The two new deadline guards leave a seam: the entry validation rejects only non-finite --timeout/--budget, and this clamp silently converts finite non-positive deadlines into fake 1 ms timeouts instead of the descriptive rejection the validation establishes (probe-confirmed through the real CLI parsing). — Failure scenario: qwen review build-test --timeout -5 --budget 600 passes the finite check → the command is killed instantly at a 1 ms deadline → the report records a command that was never given any time as a timed-out infrastructure result, misframing invalid operator input (the review's data is then quoted as evidence of a flaky environment). Output stays disclosed and never blames the PR, so this is not Critical.
Suggested fix: extend the entry validation to reject negatives alongside non-finite values (e.g. --timeout must be a positive number of seconds), or, if clamping zero/negative to 1 ms is deliberate (the zero case is test-pinned), say so in the validation comment and reject only negatives.
中文说明
两个新的 deadline 守卫留有一条缝隙：入口校验只拒绝非有限的 --timeout/--budget，而此处的取整会把有限的非正 deadline 悄悄变成假的 1ms 超时，而不是像校验所确立的那样给出描述性拒绝（已用真实 CLI 解析探针确认）。— 失败场景：qwen review build-test --timeout -5 --budget 600 通过有限性检查 → 命令以 1ms deadline 被立即杀掉 → 报告把一个从未获得任何时间的命令记录为「超时的基础设施结果」，将无效的操作输入错误定性（之后 review 的数据还会被引用为「环境不稳定」的证据）。输出仍然如实披露且从不归咎 PR，因此不是 Critical。
修复建议： 把入口校验扩展为同时拒绝负数与非有限值（例如 --timeout must be a positive number of seconds）；或者，如果把零/负数取整为 1ms 是有意为之（零的情况已有测试钉住），请在校验注释中说明，只拒绝负数。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // rejecting it; NaN defeats every budget-floor comparison and reaches | ||
| // spawnSync as an invalid deadline — ERR_OUT_OF_RANGE with no report. | ||
| // Reject both flags at the one boundary every call crosses. | ||
| if (!Number.isFinite(args.timeout)) { |
There was a problem hiding this comment.
[Suggestion] R1-14: A finite-but-huge --timeout overflows the seconds→ms multiply to Infinity, which passes both new guards — Number.isFinite(1e308) is true and the clamp at line 223 passes Infinity through — reproducing exactly the ERR_OUT_OF_RANGE-with-no-report failure the clamp's own comment says it exists to prevent (probe-reproduced on node v22.23.1). — Failure scenario: --timeout 1e308 (any value ≳ 1.8e305) passes entry validation → spawnSync({timeout: Infinity}) throws → the handler emits the raw Node message with exit 1 and no JSON report — the precise outcome both new guards' comments claim to have eliminated. An explicit finite --budget masks it. The trigger is absurd operator input, but the guard advertises exactly this seal and leaves it open.
| if (!Number.isFinite(args.timeout)) { | |
| if (!Number.isFinite(args.timeout) || !Number.isFinite(args.timeout * 1000)) { |
中文说明
有限但极大的 --timeout 会在「秒→毫秒」乘法中溢出为 Infinity，它能通过两个新守卫——Number.isFinite(1e308) 为 true，且 223 行的取整会把 Infinity 原样放行——恰好复现了取整注释声称自己要消除的「ERR_OUT_OF_RANGE 且无报告」故障（已在 node v22.23.1 上用探针复现）。— 失败场景：--timeout 1e308（任何 ≳ 1.8e305 的值）通过入口校验 → spawnSync({timeout: Infinity}) 抛错 → handler 只输出 Node 原始错误信息并以 exit 1 结束，没有任何 JSON 报告——正是两个新守卫注释声称已经消除的结局。显式传入有限的 --budget 会将其掩盖。触发条件是荒谬的操作输入，但守卫既然宣称了这道密封，就不该留着这条缝。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if (applicable.length > 1) { | ||
| // Unreachable with one registered adapter, and deliberately kept: the | ||
| // selection contract is "exactly one, or nothing", and the second |
There was a problem hiding this comment.
[Suggestion] R1-10: The applicable.length > 1 ambiguity branch — the only code enforcing the "exactly one, or nothing" selection contract at the entry point — is pinned by zero assertions at the runBuildTest level: deleting the entire block keeps the suite green, because no test registers a second adapter (the selector-level test pins only selectToolchainAdapter's return shape). Probe flip proven: with a stub second adapter pushed into the registry, deleting this branch makes the same root return toolchain: 'npm' and execute the full npm flow — exactly the npm-guessing outcome this branch's own comment refuses. — Failure scenario: a cleanup PR deletes the documented-as-unreachable branch (green suite blesses it) → later, when the second adapter lands and both apply at a mixed root, the ambiguity handoff is silently replaced by npm guessing, and the second adapter's verification is discarded with no report signal.
Suggested fix: add one test that temporarily pushes a stub second adapter ({ applies: () => true, run: ... }) into toolchainAdapters (restore in afterEach), calls runBuildTest at a root both adapters claim, and asserts toolchain: 'unsupported' with the "More than one toolchain applies" note and zero exec calls.
中文说明
applicable.length > 1 歧义分支是在入口处强制执行「恰好一个，否则不选」契约的唯一代码，但在 runBuildTest 层面没有任何断言钉住它：删掉整个分支块，套件仍全绿，因为没有测试注册过第二个 adapter（selector 层的测试只钉住 selectToolchainAdapter 的返回形状）。探针翻转验证：向注册表 push 一个桩 adapter 后，删除该分支会让同一个根返回 toolchain: 'npm' 并完整执行 npm 流程——正是该分支注释明确拒绝的「由 npm 猜测」结局。— 失败场景：某个清理 PR 删掉这个「文档注明不可达」的分支（全绿套件予以认可）→ 之后第二个 adapter 落地、两者在混合根上同时适用时，歧义交接被悄悄替换为 npm 猜测，第二个 adapter 的验证被丢弃且报告毫无信号。
修复建议： 加一个测试：临时向 toolchainAdapters push 一个桩 adapter（{ applies: () => true, run: ... }，afterEach 中恢复），在两个 adapter 都声称适用的根上调用 runBuildTest，断言 toolchain: 'unsupported'、note 为 「More than one toolchain applies」 且零次 exec 调用。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| results.widenedWith = [...widened]; | ||
| frameInstallFailure(); | ||
| return results; |
There was a problem hiding this comment.
[Suggestion] R1-3: The extension of install-failure framing to the early returns (this build-failure return, and the build disk-preflight return at ~line 452) is new behaviour with no test: the only framing test covers the success path and asserts only substrings that the final-return framing alone would satisfy. Probe flip proven: removing the early call here fails a probe while all 81 existing tests stay green. — Failure scenario: npm ci exits 1 leaving a complete tree, then the build fails — the incident class this command's comments repeatedly cite; if a future refactor drops the two early frameInstallFailure() calls, every existing test stays green → the note silently loses the "report it as informational, never as a Critical" steering half — the exact mis-steer the change was made to prevent.
Suggested fix: add a test: install exec returns exitCode: 1 with a complete-tree marker, build exec returns non-zero; assert rep.note contains both the build-failure correlation text and never as a Critical. Optionally the same for the build disk-preflight return.
中文说明
把安装失败定性文案扩展到提前返回路径（此处的构建失败返回，以及约 452 行的构建磁盘预检返回）是没有测试的新行为：唯一的定性测试只覆盖成功路径，且断言的子串仅靠最终返回的定性就能满足。探针翻转验证：删除此处的提前调用后探针失败，而现有 81 个测试仍全绿。— 失败场景：npm ci 以 1 退出但留下完整依赖树，随后构建失败——正是本命令注释反复引用的事故类型；若未来重构丢掉两处提前的 frameInstallFailure() 调用，所有现有测试仍全绿 → note 会悄悄丢失「作为信息项上报、绝不作为 Critical」这半段引导——正是该改动要防止的错误引导。
修复建议： 补一个测试：install exec 返回 exitCode: 1 且带完整树标记，build exec 返回非零；断言 rep.note 同时包含构建失败关联文案与 never as a Critical。可选地对构建磁盘预检返回做同样覆盖。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| 3. Selects the sole applicable built-in adapter, failing closed to the | ||
| unsupported report when zero or more than one apply. |
There was a problem hiding this comment.
[Suggestion] R1-16: The documented command-boundary flow (and the Goals bullet "Move npm repository detection … behind the npm adapter") omits that on the zero-adapters path build-test.ts keeps its own npm-shapedness detection and calls the npm adapter directly, outside the registry: if (existsSync(join(root, 'package.json'))) return npmToolchainAdapter.run(runArgs);. — Failure scenario: the numbered contract says zero adapters → unsupported report, full stop, and claims detection moved behind the adapter → a second-adapter author implementing against it emits the generic note for zero-adapter roots and drops npm's precise handoff note for every unscopable npm root (unmodeled glob, empty glob, script-less package.json) — exactly the note-quality property the renamed test in this PR pins.
Suggested fix: add the delegation step to the command-boundary list (zero adapters + root package.json → delegate the handoff to the npm adapter for its precise note; zero adapters + no package.json → generic unsupported report), and qualify the Goals bullet to say npm applicability detection moves behind the adapter while the facade retains the npm-shaped fallback routing.
中文说明
文档记载的命令边界流程（以及 Goals 中「把 npm 仓库检测移到 npm adapter 之后」一条）遗漏了：在零 adapter 路径上，build-test.ts 保留了自己的 npm 形态检测，并在注册表之外直接调用 npm adapter：if (existsSync(join(root, 'package.json'))) return npmToolchainAdapter.run(runArgs);。— 失败场景：编号契约说零 adapter → unsupported 报告，到此为止，并声称检测已移入 adapter → 第二个 adapter 的作者照此实现时，会对零 adapter 的根输出通用 note，丢掉所有无法 scope 的 npm 根（未建模 glob、空 glob、无脚本 package.json）上 npm 的精确交接 note——正是本 PR 重命名测试所钉住的 note 质量属性。
修复建议： 在命令边界清单中补上委托步骤（零 adapter + 根有 package.json → 委托 npm adapter 产出精确 note 的交接；零 adapter + 无 package.json → 通用 unsupported 报告），并限定 Goals 的措辞：npm 适用性检测移入 adapter，facade 保留 npm 形态的兜底路由。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| Command execution, output trimming, timeout detection, and environment shaping | ||
| remain shared exports from the command module in P0 because adjacent review | ||
| commands and existing tests consume them. The npm-specific dependency widening |
There was a problem hiding this comment.
[Suggestion] R1-15: The doc states command execution "remain[s] shared exports from the command module … because adjacent review commands and existing tests consume them", but the executor run is module-private (no export, unchanged context line), nothing imports it (test-delta.ts imports only buildRunEnv, spawnTimedOut, trimOutput), and the next paragraph contradicts it: "It does not import the command's runtime executor … the command selects the adapter and passes execution in." — Failure scenario: a Maven-phase author implementing against "Shared execution primitives" expects an importable executor carrying the spawn behavior — including the fractional-deadline clamp this PR adds inside private run — and, finding none, re-implements spawn/env-shaping in the new adapter → forks the deadline-coercion and timeout-as-data semantics the doc claims are shared.
Suggested fix: rewrite as "Output trimming, timeout detection, and environment shaping remain shared exports … Command execution stays private to the command module and reaches the adapter through the injected exec argument."
中文说明
文档声称命令执行「仍是命令模块的共享导出……因为相邻 review 命令和现有测试会消费它们」，但执行器 run 是模块私有的（无 export，未改动的上下文行），没有任何模块导入它（test-delta.ts 只导入 buildRunEnv、spawnTimedOut、trimOutput），且下一段与之矛盾：「It does not import the command's runtime executor … the command selects the adapter and passes execution in.」。— 失败场景：Maven 阶段的作者按「Shared execution primitives」实现时，会期待一个可导入、带 spawn 行为的执行器——包括本 PR 在私有 run 中新增的小数 deadline 取整——发现没有后便在新 adapter 里重新实现 spawn/环境构造 → 使文档声称共享的 deadline 强制与「超时即数据」语义发生分叉。
修复建议： 改写为 「Output trimming, timeout detection, and environment shaping remain shared exports … Command execution stays private to the command module and reaches the adapter through the injected exec argument.」
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ## Files | ||
|
|
||
| P0 changes: |
There was a problem hiding this comment.
[Suggestion] R1-7: The P0 changes: file list omits packages/cli/src/commands/review/lib/disk.ts, a module this PR creates (the moved free-disk floors and statfsSync preflight that npm-toolchain.ts imports from ./disk.js). — Failure scenario: the doc is the refactor's committed record — its Status section says the next adapter phase appends its own section, and its Testing section demands the disk behavior stay unchanged — yet the module owning that behavior has no entry → a future phase author auditing the extraction against this list gets no design-level trace that the disk floors left the command module.
Suggested fix: add a bullet: packages/cli/src/commands/review/lib/disk.ts — owns the free-disk floors and the statfsSync preflight shared by the install and build phases.
中文说明
P0 changes: 文件清单遗漏了 packages/cli/src/commands/review/lib/disk.ts——本 PR 新建的模块（搬过来的磁盘空间下限与 statfsSync 预检，npm-toolchain.ts 从 ./disk.js 导入）。— 失败场景：该文档是重构的存档记录——其 Status 一节说下一个 adapter 阶段会在此追加自己的章节，Testing 一节要求磁盘行为保持不变——但承载该行为的模块却没有条目 → 未来阶段的作者对照此清单审计搬运时，得不到任何设计层面的线索表明磁盘下限已离开命令模块。
修复建议： 补一条：packages/cli/src/commands/review/lib/disk.ts —— 承载安装与构建阶段共享的磁盘空间下限与 statfsSync 预检。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| 2. A single-root npm package selects the npm adapter. | ||
| 3. A non-npm repository produces the existing unsupported report. | ||
| 4. An unmodeled npm layout remains unsupported rather than returning a false |
There was a problem hiding this comment.
[Suggestion] R1-13: This Testing item claims a non-npm repository "produces the existing unsupported report", but for roots with no package.json at all the PR changes the note text — runtime A/B confirmed byte-different reports: base emits 'No npm package here to scope (no workspaces, and the root has no build/test script)…'; the PR emits the newly invented 'No supported npm project here to scope…'. The new test named keeps the complete generic unsupported report when no adapter applies pins the changed text under a name that claims preservation, contradicting the Goals' "Preserve … all npm behavior". No consumer string-matches the note, so blast radius is agent-facing prose plus doc/test-name accuracy. — Failure scenario: byte-different report content for the no-package.json repo class contradicts the preservation goal → a future phase author auditing against this list sees green-plus-"keeps" and concludes nothing drifted.
Suggested fix: either emit the pre-PR note string verbatim from the generic fallback (restoring byte-compat), or amend the doc's Goals/Testing wording to say the no-package.json note was rephrased and rename the test so it no longer claims to "keep" the existing report.
中文说明
该 Testing 条目声称非 npm 仓库会「产生既有的 unsupported 报告」，但对完全没有 package.json 的根，本 PR 改变了 note 文案——运行时 A/B 确认报告字节级不同：基线输出 'No npm package here to scope (no workspaces, and the root has no build/test script)…'；PR 输出新造的 'No supported npm project here to scope…'。名为 keeps the complete generic unsupported report when no adapter applies 的新测试用声称「保持」的名字钉住了改变后的文案，与 Goals 中「保留……所有 npm 行为」矛盾。没有消费方对该 note 做字符串匹配，因此影响范围是面向 agent 的文案加文档/测试名准确性。— 失败场景：无 package.json 仓库类别的报告内容字节级不同，违背保留目标 → 未来阶段作者对照此清单审计时看到全绿加「keeps」，会得出「没有漂移」的结论。
修复建议： 要么让通用兜底逐字输出 PR 前的 note 字符串（恢复字节级兼容），要么修订文档 Goals/Testing 的措辞说明无 package.json 的 note 被改写，并给测试改名使其不再声称「保持」既有报告。
— qwen3.8-max via Qwen Code /review (v0.21.8)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): lib/npm-toolchain.ts — no such file or directory; lib/disk.ts — no such file or directory; lib/npm-toolchain.test.ts — no such file or directory; 2293 tests passed — this review observed 18753 passed.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan（非阻断）：lib/npm-toolchain.ts — no such file or directory; lib/disk.ts — no such file or directory; lib/npm-toolchain.test.ts — no such file or directory; 2293 tests passed — this review observed 18753 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 1/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31383566918 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Test Plan (not a blocker): lib/npm-toolchain.ts — no such file or directory; lib/disk.ts — no such file or directory; lib/npm-toolchain.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 Test Plan（非阻断）：lib/npm-toolchain.ts — no such file or directory; lib/disk.ts — no such file or directory; lib/npm-toolchain.test.ts — no such file or directory。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
|
|
||
| export interface BuildTestReport { | ||
| /** `npm` when the workspace scoping applied; `unsupported` otherwise. */ | ||
| /** The scoped toolchain that ran, or `unsupported` when selection was unsafe. */ |
There was a problem hiding this comment.
[Suggestion] R1-1: Hunk survived — JSDoc comment update on BuildTestReport.toolchain has no test coverage. The comment is not mechanically checked by any test, so a future change that mis-syncs the comment would not be caught.
中文说明
[Suggestion] R1-1：代码块存活——BuildTestReport.toolchain 的 JSDoc 注释更新没有测试覆盖。该注释没有被任何测试机械地检查，因此未来修改注释时不会发现其与代码不一致。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the changed unit suite ran on Linux only. Test Plan (not a blocker): lib/npm-toolchain.ts — no such file or directory; lib/disk.ts — no such file or directory; lib/npm-toolchain.test.ts — no such file or directory.
中文说明
已审查。 未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; the changed unit suite ran on Linux only。 Test Plan（非阻断）：lib/npm-toolchain.ts — no such file or directory; lib/disk.ts — no such file or directory; lib/npm-toolchain.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 2/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31389561905 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. 3 of those were silent-sandbox (idle) timeouts that no budget increase can cure — investigate the sandbox image and runner docker daemon for those. A human should investigate the sandbox image and runner docker daemon, then comment What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31394772225 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停：本 PR 达到轮次上限（100/100）。评论 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 82 passed · 0 failed · 82 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：82 通过 · 0 失败 · 82 总计 Verification reportPR #8776 Deep Verification — refactor(review): extract the toolchain adapter boundaryVerdict: 中文摘要结论：
Central claim and A/B proofCentral claim: moving the npm implementation behind Method: compiled
Live capture of the cells as they printed: The six measured deltas — each contradicts the description's "every npm behaviour are unchanged". Five of the six are pinned by the PR's new tests (mutation matrix below); D4's wording is not (see Findings):
D5 and D6 are bug fixes to report honesty (base could tell the agent a suite ran when it did not). D1–D4 are hardening/disclosure. All six change the report a downstream consumer reads in the named edge states — see Findings. Corrections
FindingsLow — six behavior fixes ship inside a PR described as a pure move (D1–D6 above). Not a defect: every delta is an improvement and five of six have a dedicated test. The consequence a reviewer should accept knowingly: Nit — D4's wording/placement is untested (classified above as a coverage gap; the behavior itself is proven correct by cell s04). A one-line assertion in Nit — description test evidence is stale (see Correction 3). No higher-severity findings. The A/B found no drift in any preserved scenario; the mutation matrix found no survivor among the eight targeted guards (the one unpinned axis, D4's wording, had no suite assertion to target — classified above); the lint gate (proven live with a planted unused variable) is clean on all six changed/added files; the bundled Mutation matrix (new guards → pinned?)Baseline for the two pinned files: 2 files, 81 tests green. Each mutant applied to head source, suite rerun, tree restored (
8/8 killed, zero survivors, each kill attributed to the test the commit added for that guard. Capture: The one unpinned axis found this round is D4's wording/placement: reverting "so the run went ahead" to base's phrasing — or dropping the two early-return Targeted gates
Not covered
MethodologyRan in the CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Confidence: 4/5 — a careful, well-pinned move that checks out end to end; the residual nits are about description wording and suggestion-level pinning, not the code. Stepping back: my independent baseline for this goal was "extract the npm algorithm into one module and keep dispatch inline until Maven exists." This PR goes one step further — a 51-line contract with fail-closed selection — and it earns that step: the Maven phase is a declared, documented next step rather than a hypothetical, and the subtle piece is done right. The delegation that keeps npm's precise handoff notes for unscopable-but-npm-shaped roots is exactly the kind of detail that would have been lost in a naive extraction, and I verified its safety myself: The parts that give me confidence: the move is verbatim except five intentional fixes, and every one of them is commented and has a dedicated test — including a test that pins the false-green it removes. The exported seams survive with an identity test, so consumers cannot silently drift. Three The nits, named so they don't get lost: the description's "every npm behaviour is unchanged" overstates slightly (corner-case note text changed, boundary validation added — all for the better, but a reviewer should know the diff is not purely mechanical), and the design doc carries four small inaccuracies the review annotated. Neither changes what the code does. In six months, the Maven phase either lands as a registration — the doc and the selection contract are ready for it — or someone deletes a 51-line file. Both outcomes are cheap. That is the right trade. Verdict: approve. CI is fully green and settled on the reviewed commit; approval pinned to it. 中文说明置信度：4/5——一次谨慎、测试钉扎到位的搬运，端到端核对无误；遗留的小问题在于描述措辞和建议级的测试覆盖，而非代码本身。 退一步看：我为这个目标设定的独立基线是"把 npm 算法抽到一个模块，在 Maven 真正存在之前保持内联分发"。本 PR 多走了一步——51 行契约加 fail-closed 选择——而这一步是值得的：Maven 阶段是已声明、已写入设计文档的下一步，而非假设；且微妙的部分做对了。对"npm 形态但无法 scope"的根目录委托 npm adapter 保留精确 handoff note，正是简单粗暴的抽取会丢掉的细节，我亲自验证了它的安全性： 让我有信心的部分：除了五处有意为之的修复外，搬运是逐行原样的，且每处修复都有注释和专门测试——包括一个钉住其所消除的假绿的测试。导出接缝以同一性测试保留，消费方不会悄悄漂移。三轮 点名小问题以免遗失：描述中"所有 npm 行为未变"略有夸大（角落场景的 note 文案有变化、新增了边界校验——都是好的变化，但 reviewer 应知道 diff 并非纯机械搬运）；设计文档有四处评审已标注的小不准确。两者都不改变代码的实际行为。 六个月后，Maven 阶段要么以一次注册落地——文档与选择契约已为此就绪——要么有人删掉一个 51 行的文件。两种结局代价都很低。这是正确的取舍。 结论：批准。被审 commit 上 CI 全绿且已结束；批准固定在该 commit。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The move claim holds: 719 of 749 removed non-blank lines reappear verbatim, and the npm algorithm (install policy, workspace selection, build ordering, dependency widening, reverse-dependency closure, timeout/disk classification, build-only path) is byte-identical otherwise. The seven semantic deltas are all improvements and all test-pinned (the timeout clamp fixes a real latent ERR_OUT_OF_RANGE crash from fractional spawnSync timeouts; Number.isFinite guards; unfiltered budget-floor notRun; single-root budget-stop note; early install-failure framing). Selection is correct: exactly-one-applicable with fail-closed unsupported on zero-or-several, the zero-adapter npm-shaped-root delegation preserves the precise npm handoff note, applies() is equivalent to the old in-algorithm detection across all layout classes, and no command executes before any gate. BuildTestReport/CommandResult field lists and the stdout/--out/exit-code contract are byte-identical base vs head, and the consumers (base-tree.ts, test-delta.ts) read only intact fields — the autofix gate cannot drift.
Non-blocking: one P2 worth a follow-up — --timeout 1e308 passes Number.isFinite, overflows perCommandMs to Infinity, and spawnSync({timeout: Infinity}) throws ERR_OUT_OF_RANGE with no JSON report (pre-existing on base, strictly improved here, pathological input only; clamp to a sane max or reject huge values at entry). P3s: the PR body's 'every npm behaviour unchanged' slightly overclaims the seven pinned deltas; docs/design/review-toolchain-adapters.md still omits lib/disk.ts, the zero-adapter npm delegation, and two note-wording changes — worth fixing since the next adapter phase builds on that doc. CI green on head. Ship it.
|
Released in v0.21.10. |
5 participants
What this PR does
Moves the npm implementation of
qwen review build-testbehind an internal toolchain adapter contract.build-test.tskeeps CLI routing, plan reading, output trimming, env shaping, and the spawn boundary;lib/npm-toolchain.tsowns npm detection and the verification algorithm;lib/disk.tsholds the shared free-disk floors;lib/toolchain.tsdefines the contract and selects the sole applicable adapter.This is a move, not a rewrite. 95% of the non-blank lines removed from
build-test.ts(718 of 749) reappear verbatim in the new files. The CLI arguments, theBuildTestReportJSON shape, and every npm behaviour are unchanged.Why it's needed
build-test.tscombined three responsibilities in one module: reading the review plan, deciding which repository toolchain can be verified deterministically, and implementing npm workspace installation, affected-package selection, dependency widening, build execution, test execution, and result reporting.Adding a second toolchain directly to that module would grow a conditional command rather than a verification boundary, and would make the existing npm behaviour harder to protect while it grows. Extracting the boundary first keeps that follow-up reviewable as its own change.
Selection requires exactly one applicable adapter and fails closed to the
unsupportedhandoff when zero or several apply, so a second toolchain lands as a registration rather than another branch in this file.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/— expect all files to pass.build-test.test.tsis the compatibility oracle: it exercises the report throughrunBuildTestand was not rewritten for the new file layout.git show --stat HEADshowsbuild-test.tsshedding whatlib/npm-toolchain.tsandlib/disk.tsgain. Reading the two side by side, the npm algorithm is unchanged — install policy, workspace selection, build ordering, dependency widening, the reverse-dependency closure and its caveats, timeout and disk classification, and the build-only path.BuildTestReport.toolchainstays'npm' | 'unsupported', and the exported seamsrunBuildTest,trimOutput,buildRunEnv,spawnTimedOut, andunresolvedWorkspaceDepsare still exported frombuild-test.ts.unsupportedhandoff carrying npm's precise reason rather than a generic one (lib/npm-toolchain.test.ts).Evidence
packages/clireview suite: 65 files passed, 2293 tests passed, 6 skipped. Prettier and ESLint clean on the changed files.tsc --noEmitreports no errors undersrc/commands/review/; the errors it does report are pre-existing and confined toserve/andacp-integration/(an unbuiltpackages/acp-bridge/distin a fresh worktree), untouched here.Tested on
Risk & Scope
build-test.test.tsas an unmodified-in-substance oracle and by the 95% verbatim-line measurement.test-efficacy, which stays npm-workspace and Vitest specific.Linked Issues
Supersedes the first commit of #8394, which is being split into reviewable pieces.
中文说明
此 PR 的改动
把
qwen review build-test的 npm 实现移到内部 toolchain adapter 边界之后。build-test.ts保留 CLI 路由、plan 读取、输出裁剪、环境构造和 spawn 边界；lib/npm-toolchain.ts承担 npm 检测与验证算法；lib/disk.ts存放共享的磁盘空间下限；lib/toolchain.ts定义契约并选出唯一适用的 adapter。这是搬运，不是重写。 从
build-test.ts删除的非空行中有 95%（749 行中的 718 行）逐行原样出现在新文件里。CLI 参数、BuildTestReport的 JSON 结构和全部 npm 行为均未改变。为什么需要
build-test.ts把三件事混在一个模块里：读取 review plan、判断哪个 toolchain 可以确定性验证、以及实现 npm workspace 安装、受影响包选择、依赖扩展、构建执行、测试执行和结果上报。直接往这个模块里加第二个 toolchain，得到的是一个不断膨胀的条件分支，而不是一条验证边界，同时会让现有 npm 行为在膨胀过程中越来越难保护。先抽出边界，可以让后续工作作为独立改动被 review。
adapter 选择要求恰好一个适用；零个或多个时 fail closed 到
unsupported交接。这样第二个 toolchain 是一次注册，而不是本文件里的又一个分支。Reviewer Test Plan
验证方式
cd packages/cli && npx vitest run src/commands/review/，预期全部通过。build-test.test.ts是兼容性判据：它通过runBuildTest检验报告，并未为新的文件布局重写。git show --stat HEAD显示build-test.ts减少的部分正是lib/npm-toolchain.ts与lib/disk.ts增加的部分。对照阅读可见 npm 算法未变 —— 安装策略、workspace 选择、构建顺序、依赖扩展、反向依赖闭包及其 caveat、超时与磁盘分类、build-only 路径。BuildTestReport.toolchain仍为'npm' | 'unsupported'，且runBuildTest、trimOutput、buildRunEnv、spawnTimedOut、unresolvedWorkspaceDeps仍从build-test.ts导出。unsupported交接，并带上 npm 自己的精确原因而非泛化提示（lib/npm-toolchain.test.ts）。证据
packages/clireview 套件：65 个文件通过，2293 个测试通过，6 个跳过。改动文件的 Prettier 与 ESLint 均通过。tsc --noEmit在src/commands/review/下无错误；其报告的错误均为既有问题且集中在serve/与acp-integration/（全新 worktree 中未构建的packages/acp-bridge/dist），本 PR 未触及。风险与范围
build-test.test.ts作为实质未改的判据保留，以及 95% 逐行一致的度量。test-efficacy（仍然是 npm workspace + Vitest 专用）。