feat(review): provision worktree dependencies from a host cache on the persistent runners - #10129
feat(review): provision worktree dependencies from a host cache on the persistent runners#10129wenshao wants to merge 9 commits into
Conversation
…e persistent runners The review worktree fetch-pr builds is a bare checkout, and a full npm ci plus the prepare build does not fit inside a review agent's tool budget, so every probe that decided the right evidence was "run the test" burned its budget on a doomed install and the round downgraded to a disclosed read-only audit. On the persistent pool the install now happens once, in a workflow step outside every agent's budget: the step keeps $HOME/.qwen-review-deps/<lockfile-sha256> populated (a real npm ci result — root and nested node_modules, every workspace's built dist, npm's own completeness marker — snapshotted by hardlink and renamed into place atomically), and fetch-pr link-farms the entry matching the PR's own lockfile into the worktree it creates. Workspace self-links are re-derived against the worktree's members so a probe never tests the base's copy of a changed sibling; dist is copied, not linked, so a scoped rebuild cannot write through into the shared cache; npm's completeness marker is placed only on a clean farm, so build-test's install gate stays honest. A cold cache, a diverged lockfile, or a farm that could not complete falls back to today's behaviour with the reason recorded in the plan report. exposeDependencies' containment learns the provisioned shape: a scratch tree farmed from a provisioned worktree finds entries resolving into the cache, and without the validated provenance marker they would all count as escapes and the scratch tree would start with no dependencies at all. Fixes #10108
github-actions
Bot
added
the
review/self-reported
… the workflow size ratchet The workflow-size gate refuses qwen-code-pr-review.yml growing 7188 bytes past its recorded baseline (allowance 4096), and its own guidance prefers moving long steps into .github/scripts/ over bumping the number. The step body moves verbatim into provision-review-deps.sh (shebang and narrative header added, executable bit set — the step invokes it by bare path); the YAML keeps a thin invocation with the gate conditions, and the test suite now reads the script file, pins that the step actually invokes it, and pins the executable bit the scenario harness cannot see.
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical — issue #10108 documents review rounds that burned their tool budget on a doomed install and downgraded to disclosed read-only audits (PR #9729 rounds 13/15; PR #9940's own review could not run the tests it added). That is the strongest kind of motivation for this pipeline. Direction: the repo's own review infrastructure on the persistent ECS pool — letting Size: ~787 production lines (workflow step 117, Approach: matches what I would have proposed independently — a lockfile-hash-keyed host cache populated outside any agent's budget, link-farmed into the worktree, fail-open throughout. The three deliberate semantics (self-links re-derived against the worktree, Risk: no elevated signal — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到的问题，不是理论性的——issue #10108 记录了评审轮次把工具预算烧在注定失败的安装上、降级为披露式只读审计（PR #9729 第 13/15 轮；PR #9940 的评审跑不了它自己新增的测试）。对这条流水线来说这是最强的动机类型。 方向： 仓库自身在持久 ECS 池上的评审基础设施——让 /review 的探针能跑测试而不是降级只读。完全在维护项目自身流水线的范围内；不触及产品面，CHANGELOG 方向信号在此不适用。 规模： 最初 8 个文件，生产逻辑约 787 行（workflow 步骤 117、dep-provision.ts 572、fetch-pr.ts 56、worktree.ts 38、内置 skill 文档 4）加 685 行测试。低于 1,000 行建议线。唯一触及核心路径的是内置评审 SKILL.md（+2/−2，纯文档）；作者是这条流水线的维护者、也是关联 issue 的作者，故不做额外升级——明细记录在此备查。 方案： 与我独立设想的方案一致——按锁文件哈希键控的宿主缓存、在 agent 预算之外填充、用链接农场映进 worktree、处处 fail-open。三个刻意语义（self-link 按 worktree 重新推导、dist 复制而非链接、完整性标记只放在干净农场上）恰好是这个设计的三个陷阱，且各有处理。略读时的一个预警后来被证实：内联步骤让 workflow YAML 增长约 7KB，而本仓库有尺寸棘轮——评审进行中落地的后续提交 3fa88cf 把步骤正文移入 .github/scripts/provision-review-deps.sh，正是棘轮推荐的形态。 风险： 无升级信号——改动文件均未命中与 revert 相关的路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRead both commits: the original eight-file change at What I verified against the base code, not just the description:
Two non-blocking follow-ups:
sequenceDiagram
participant P1 as Populate step (before any agent)
participant P2 as Host cache entry
participant P3 as fetch-pr
participant P4 as Review worktree
participant P5 as build-test and probes
P1->>P2: npm ci + prepare build, hardlink snapshot, completeness marker last
P3->>P2: select entry by the PR's own lockfile hash
P3->>P4: link-farm node_modules, copy prebuilt dist, re-derive self-links
P5->>P4: marker present, install skipped, tests run
Files changed (9 of 9)
Testing evidence — the PR's own CI, via the APIThe first commit (
That gate killed the ubuntu Test job before any test ran, so the first commit had no CI test evidence at all (the macOS/Windows/integration lanes never started; the coverage-comment failure was the downstream artifact miss). The follow-up commit On the reviewed commit Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The author reports the 16 provisioner cases, the two containment cases, and the 10 workflow-bash cases passing locally on Linux — that is the author's claim, and it is plausible given the test code, but it is not evidence until this commit's CI run lands; the gate failure above shows why that distinction matters. Once green, the suite genuinely pins the change: every one of these tests is new with the diff, so a passing run cannot be a suite that passes identically without it. What no pre-merge lane can settle — including a sandboxed Real-scenario (tmux) drive: N/A on this unattended CI run, and the change has no user-visible TUI surface — it is CI infrastructure for the review pipeline. 中文说明代码审查两个提交都读了：8fe8c660 的八文件改动，以及评审过程中 head 移动后以增量 diff 形式审查的后续提交 3fa88cf。 对照 base 代码验证过的要点：build-test 的安装闸门确实是 node_modules/.package-lock.json——正是农场放置的标记，且只放在零失败的农场上；条目选择以 worktree 自己的锁文件为键（先哈希、再与条目内副本逐字节比较），PR 改锁文件时按构造回退到旧行为；不可信输入的处理与姊妹农场已有的 containment 规则一致（成员目录双侧实路径包含、self-link 名称逃逸检查、committed 的 node_modules/dist 拒绝覆盖、provenance 标记先验证后信任）；fail-open 是彻底的（步骤层 continue-on-error 加超时、脚本内 set +e 加收尾 exit 0、供给器记录原因而不抛异常）；后续提交干净——脚本正文与我审过的 YAML 版本逐字一致，接线由测试钉死（step.run 必须等于脚本路径），可执行位有独立测试。 两个不阻塞的后续项：新增的 bash 场景用例执行 GNU 专有工具（sha256sum、mv -T、touch -d @epoch、find -printf），该套件只在 Windows 上被排除，而 macOS/Windows PR 通道目前全仓库关闭，所以接下来跑它们的是 main 的 nightly——很可能在 macOS 上失败。文件里已有现成模式（it.skipIf(!hasGnuMktemp) 加"评审池仅 Linux"的理由注释），且生产步骤只在 Linux 池运行，故不阻塞合并——但值得补上这个守卫，避免 nightly 为一个步骤从不运行的宿主变红。小建议：mkdirSync(join(linkPath, '..')) 写作 dirname(linkPath) 更清晰。 时序图见正文（填充步骤 → 缓存条目 → fetch-pr 选取并农场化 → build-test 与探针免安装跑测试）。 测试证据首个提交 8fe8c66 的 CI 因 PR 自身原因失败：尺寸棘轮拒绝（YAML 超出记录基线 7188 字节， allowance 4096），闸门在任何测试运行前就终止了 job，因此该提交没有任何 CI 测试证据。后续提交 3fa88cf 采用棘轮推荐的修复方式；经 API 实测该提交的 workflow 文件为 159,471 字节，较记录基线 158,010 仅增 1,461，在 4,096 allowance 内，闸门应通过。 在受审提交 3fa88cf 上 CI 仍在运行——只抓取一次、不轮询，finalize job 会在结束后更新上方表格。作者在 Linux 本地报告各套件通过——这是作者的声明，合理但尚非证据，等本次 CI 落地才算；一旦变绿，套件是真正钉住改动的（所有测试都随 diff 新增）。任何合并前的通道（包括沙箱 /verify）都无法预先验证 ECS 池上的首次真实填充——它按设计是合并后可观测的，PR 描述里已给出要盯的日志行。 真实场景（tmux）驱动：本次无人值守 CI 运行不适用；且该改动无用户可见的 TUI 面——是评审流水线的 CI 基础设施。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — solid design, careful implementation, and the one hard problem this review found was already fixed in-flight; the two remaining items are non-blocking follow-ups. Stepping back: this solves a problem I can name from the repo's own review history — probes that wanted to run a test burned their budget on a doomed install and the round downgraded to a read-only audit. The approach matches the one I would have proposed before reading the diff, and where it deliberately differs from the naive version (self-links re-derived against the worktree, The workflow-size ratchet failure on the first commit was the one genuine blocker, and What keeps this at 4 rather than 5:
Verdict: approve — deferred until CI lands green on the reviewed commit, per the marker below; the finalize job posts the commit-pinned approval if everything comes back green, and withholds it if anything lands red or the head moves. 中文说明置信度：4/5 —— 设计扎实、实现细致，本次审查发现的唯一硬问题已在评审过程中修复；剩余两项均为不阻塞的后续事项。 退一步看：这个 PR 解决的问题能从仓库自己的评审历史里点名——探针想跑测试却把预算烧在注定失败的安装上，轮次降级为只读审计。方案与我读 diff 前独立设想的一致；凡是刻意偏离朴素版本的地方（self-link 按 worktree 重新推导、dist 复制而非链接、npm 标记只放在干净农场上），都是在堵真实的陷阱，而不是仪式。对一条要农场化不可信 PR 内容的流水线，安全姿态也是对的：一切来自 PR 的东西要么验证要么拒绝，provenance 标记是校验后才信任。彻底的 fail-open 意味着任何缓存故障的最坏结果就是今天的行为，绝不构成回归。 首个提交上的尺寸棘轮失败是唯一的真实阻塞项，3fa88cfe 已按闸门自身推荐的方式修复——脚本抽出、接线由测试钉死、可执行位也有测试。六个月后维护这段代码我会感谢作者：注释在每个分支都写明了"为什么"，测试钉住了关键契约（环境变量/标记字面量保持同步、步骤顺序、绝不失败的承诺）。 停在 4 而不是 5 的原因：
结论：批准——按下方标记推迟到 CI 在该提交上变绿；若全部变绿，finalize job 会提交钉住该提交的批准，若有变红或 head 移动则不予批准。 — 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. |
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: the executable-script lint — .github/scripts/provision-review-deps.sh: shellcheck is not installed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：the executable-script lint — .github/scripts/provision-review-deps.sh: shellcheck is not installed。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| fi | ||
| HASH="$(sha256sum package-lock.json | cut -d' ' -f1)" | ||
| ENTRY="$CACHE_ROOT/$HASH" | ||
| if [ -f "$ENTRY/.qwen-review-deps-complete" ]; then |
There was a problem hiding this comment.
[Critical] The cache entry is trusted on the presence of .qwen-review-deps-complete alone, but entries live on a path writable by the unsandboxed PR code this same job executes, and nothing validates entry content. A malicious PR — or any PR test that writes under node_modules/<pkg> — can trojanize the current entry, or pre-create the entry for the current lockfile hash entirely, keeping the marker, a byte-identical package-lock.json copy and node_modules/.package-lock.json: the only things the warm check here and dep-provision.ts validate. The warm-path touch then keeps a planted entry newest for the LRU prune, so every later review of any PR with an unchanged lockfile on this runner link-farms the poisoned tree, records provisioned: true, build-test skips its install on the npm marker, and probes execute attacker-chosen bytes while reviewing unrelated PRs. Probe-verified: a pre-planted trojanized entry was farmed and served (provisioned: true, failed: 0); the real script's warm path against it exited 0 with zero npm calls; a single writeFileSync through a worktree farm link changed the shared entry's file. Suggested fix: make entries self-authenticating or immutable — write a path+sha256 manifest into the stage before the atomic rename and verify it on this warm path (rebuild, not exit, on mismatch) and in dep-provision.ts before farming, failing closed; and/or chmod -R a-w the entry after publish (restoring writability in the prune path before rm -rf). Add a workflow test asserting a manifest-mismatched entry is rebuilt instead of warm-exiting, and a dep-provision test asserting such an entry returns provisioned: false — remove the verification and confirm both go red.
中文说明
[Critical] 缓存条目仅凭 .qwen-review-deps-complete 标记的存在即被信任,但条目所在路径可被同一 job 执行的、未加沙箱的 PR 代码写入,且条目内容从不做任何校验。恶意 PR——或任何在 node_modules/<pkg> 下写文件的 PR 测试——都可以向当前条目植入木马,甚至直接为当前锁文件哈希预先创建整个条目:只需保留标记文件、一份逐字节一致的 package-lock.json 副本和 node_modules/.package-lock.json——这正是此处 warm 检查与 dep-provision.ts 校验的全部内容。warm 路径的 touch 还会让植入条目在 LRU 剪枝中保持最新,于是该 runner 上之后每个锁文件未变的 PR 评审都会链接农场化这棵被污染的树,记录 provisioned: true,build-test 因 npm 标记跳过安装,探针在评审无关 PR 时执行攻击者选定的字节。已用探针验证:预植入的木马条目被成功农场化并提供(provisioned: true, failed: 0);真实脚本的 warm 路径对其退出 0 且零次 npm 调用;通过 worktree 农场链接的一次 writeFileSync 改掉了共享条目内的文件。建议修复:让条目自证或不可变——在原子重命名前向 stage 写入路径+sha256 清单,并在此 warm 路径(不匹配时重建而非退出)和 dep-provision.ts 农场化前校验,失败即回退;或在发布后 chmod -R a-w 条目(剪枝路径 rm -rf 前恢复可写)。请补充一个 workflow 测试断言清单不匹配的条目会被重建而非 warm 退出,以及一个 dep-provision 测试断言此类条目返回 provisioned: false——删掉校验逻辑,确认两个测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const linkPath = resolve(join(worktreeReal, 'node_modules'), name); | ||
| if (!insideDir(join(worktreeReal, 'node_modules'), linkPath)) { |
There was a problem hiding this comment.
[Critical] The self-link loop's only check is endpoint containment; it performs rmSync/mkdirSync/symlinkSync on paths derived from PR-controlled member names without validating name shape. Four probe-confirmed entrances, each reporting provisioned: true with failed: 0 and handing over the npm marker: (1) a member named "." (or 'foo/..') collapses onto the farm root — the occupied branch's rmSync deletes the entire farm including the provenance marker, node_modules is re-created as a symlink to the member dir, and the npm marker is written through it; (2) a multi-segment name like "plain-pkg/x" traverses an entry-farm symlink — the mkdir/rm/symlink land INSIDE the shared host cache entry, persistently poisoning every later same-lockfile review; (3) a member named ".qwen-review-farm" deletes the farm's own provenance marker, so provisionSourceOf returns null and every scratch tree built for this review loses its dependencies; (4) a member named ".bin" (or a bare scope "@fix") replaces the farm's .bin link / scope directory, making every binary unreachable. Reachable because readWorkspacePackages admits any non-empty string name and the lockfile byte-compare passes (the PR never regenerates package-lock.json). Suggested fix: validate the name's shape before any filesystem operation — reject names npm rejects (leading . or _), reject anything that is not exactly one segment or exactly two segments with the first starting @ and none empty/./.., and reserve the farm-owned paths (.bin, .package-lock.json, .vite, .vite-temp, the provision marker); count tally.failed++ and continue. Each partial fix (shape rule alone, reserved-path list alone) leaves at least one entrance open. Add one test per entrance (modeled on the existing ../evil case, lockfile unchanged): assert provisioned === false, failed > 0, and the farm/entry/marker/.bin intact — each is red at HEAD.
中文说明
[Critical] self-link 循环的唯一检查是端点包含性;它对来自 PR 可控成员名的路径执行 rmSync/mkdirSync/symlinkSync,却从不校验名字形状。四个经探针确认的入口,全部报告 provisioned: true、failed: 0 并交出 npm 标记:(1) 成员名 "."(或 'foo/..')坍缩到农场根——占用分支的 rmSync 删掉整个农场(含 provenance 标记),node_modules 被重建为指向成员目录的符号链接,npm 标记透过它写入;(2) 多段名如 "plain-pkg/x" 穿过条目农场符号链接——mkdir/rm/symlink 落在共享宿主缓存条目内部,持久毒化之后每个同锁文件评审;(3) 成员名 ".qwen-review-farm" 删掉农场自己的 provenance 标记,provisionSourceOf 返回 null,为本评审构建的每个 scratch 树都失去依赖;(4) 成员名 ".bin"(或裸作用域 "@fix")替换农场的 .bin 链接/作用域目录,所有可执行文件不可达。可达原因:readWorkspacePackages 接受任何非空字符串名,且锁文件字节比对通过(PR 不重新生成 package-lock.json)。建议修复:在任何文件系统操作前校验名字形状——拒绝 npm 拒绝的名字(前导 . 或 _),拒绝不是恰好一段、或不是恰好两段且首段以 @ 开头且无空/./.. 段的名字,并保留农场自有路径(.bin、.package-lock.json、.vite、.vite-temp、provision 标记);计 tally.failed++ 并 continue。任何部分修复(只做形状规则、或只做保留路径清单)都至少留一个入口。请为每个入口加一个测试(仿照现有 ../evil 用例,锁文件不变):断言 provisioned === false、failed > 0、农场/条目/标记/.bin 完好——每个在 HEAD 都是红的。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| } | ||
| } | ||
|
|
||
| describe('dependency-cache step (real bash, stubbed npm)', () => { |
There was a problem hiding this comment.
[Critical] This real-bash suite drives a GNU-only script (sha256sum, mv -T, find -printf) and uses a GNU-only fixture (touch -d @epoch in the prune case) but carries no capability gate, while scripts/tests/vitest.config.ts excludes this file on win32 only — the macOS nightly lane (test_macos, merge_group/schedule/workflow_dispatch; its pull_request trigger is off, so this PR's own checks report skipped) runs it and fails: sha256sum absent → HASH empty → no entry keyed; BSD mv lacks -T → the entry never lands; the prune fixture's touch -d throws uncaught. Modeled-BSD execution of the real script reds the warm-path, cold-cache and prune assertions (sha256sum: command not found → cold path on an empty hash → npm invoked). ci.yml says "Read a red nightly as a blocker; nothing else will report it" — this is the exact class of incident #9220, and it was already raised as a follow-up in this PR's triage comments. Suggested fix: gate this describe on the host toolchain using the file's existing capability-probe convention (hasGnuMktemp/hasJq): probe sha256sum/find/mv/touch and describe.skipIf(!hasGnuDepsToolchain). The portable wiring block stays ungated. Witness: on a BSD host the three scenarios red without the gate; the lane never runs the production step (Linux-pool-only), so gating the test costs nothing.
中文说明
[Critical] 这个真实 bash 套件驱动 GNU 专有脚本(sha256sum、mv -T、find -printf),剪枝用例还用了 GNU 专有 fixture(touch -d @epoch),却没有任何能力门控;而 scripts/tests/vitest.config.ts 只在 win32 上排除本文件——macOS nightly 通道(test_macos,merge_group/schedule/workflow_dispatch 触发;pull_request 触发关闭,所以本 PR 自己的检查显示 skipped)会运行它并失败:sha256sum 缺失 → HASH 为空 → 条目无法键控;BSD mv 没有 -T → 条目无法落位;剪枝 fixture 的 touch -d 直接抛错。在 BSD 模拟下执行真实脚本,warm-path、冷缓存、剪枝断言全部变红(sha256sum: command not found → 空哈希走冷路径 → 调用 npm)。ci.yml 写明"红色 nightly 视为阻塞,没有其他东西会报告它"——这正是 #9220 事故的同类,且本 PR 的 triage 评论里已作为后续项提出。建议修复:用文件里现成的能力探测惯例(hasGnuMktemp/hasJq)按宿主工具链门控本 describe:探测 sha256sum/find/mv/touch,describe.skipIf(!hasGnuDepsToolchain)。可移植的 wiring 块保持不门控。见证:BSD 宿主上无门控时三个场景变红;该通道从不运行生产步骤(仅 Linux 池),门控测试零成本。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| members = graph.packages.map((p) => ({ dir: p.dir, name: p.name })); | ||
| // `skipped` dirs are real directories npm links whose manifests the graph | ||
| // cannot model; they still deserve their nested install and dist. | ||
| memberDirs = [...graph.packages.map((p) => p.dir), ...graph.skipped]; |
There was a problem hiding this comment.
[Suggestion] skipped workspace dirs receive nested installs and dist copies but never a self-link and never a count, so provisioned: true is claimed for trees where npm would have linked a member this farm does not. readWorkspacePackages puts two shapes in skipped that npm still links: a manifest with no usable name (npm links it under its directory basename) and the glob-ordering case (['packages/foo/nested', 'packages/*'], modeled last-match-wins while npm keeps both members). Probe at HEAD: with that glob order, graph_skipped: ['packages/foo/nested'], result provisioned: true, failed: 0, no self-link for the nested member, npm marker placed — a dependent importing it by name hits ERR_MODULE_NOT_FOUND under a tree the report calls fully provisioned, and the hole is blamed on the diff. Globs live in package.json, so a PR can introduce this without touching the lockfile. Suggested fix: link skipped dirs the way npm does (directory basename), or count each skipped dir in tally.failed so the npm marker is withheld and build-test repairs. Add a case with workspaces: ['packages/core', 'packages/*']: today provisioned: true with no link; after the fix either the link exists or provisioned: false — removing the handling turns it red.
中文说明
[Suggestion] skipped 的 workspace 目录会得到嵌套安装和 dist 复制,但从不建立 self-link、也从不计数,于是对 npm 本会链接而本农场未链接成员的树,仍声称 provisioned: true。readWorkspacePackages 会把两种 npm 仍会链接的形状放入 skipped:没有可用名字的清单(npm 按目录名链接),以及 glob 顺序情形(['packages/foo/nested', 'packages/*'],此处建模为后者覆盖,而 npm 两个成员都保留)。HEAD 探针:该 glob 顺序下 graph_skipped: ['packages/foo/nested'],结果 provisioned: true, failed: 0,嵌套成员无 self-link,npm 标记照放——依赖方按名字 import 它时命中 ERR_MODULE_NOT_FOUND,而报告称树已完整供给,漏洞被归咎于 diff。glob 在 package.json 里,PR 不动锁文件即可引入。建议修复:按 npm 的方式链接 skipped 目录(目录基名),或把每个 skipped 目录计入 tally.failed,从而扣住 npm 标记、由 build-test 修复。新增用例 workspaces: ['packages/core', 'packages/*']:今天 provisioned: true 且无链接;修复后要么链接存在、要么 provisioned: false——删掉该处理测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R1-4] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R1-4] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
| # Prune: newest three entries stay (~2G each; distinct installs share no | ||
| # inodes). Sweep stale stages too — an entry name is 64 hex chars, so | ||
| # neither pattern can touch the other's files. | ||
| find "$CACHE_ROOT" -maxdepth 1 -mindepth 1 -type d -printf '%T@ %f\n' 2>/dev/null | |
There was a problem hiding this comment.
[Suggestion] The prune's LRU signal (mtime) is only freshened for the CURRENT lockfile's entry (the warm-path touch), but the entry actually serving a review can be an older one — so the comment's claim that "an entry that is still serving reviews is never the one evicted" does not hold for entries serving concurrent reviews of PRs based before a lockfile bump: fetch-pr selects by the worktree's own lockfile and never touches the entry's mtime. A long-running review farmed from entry X; three newer entries appear (three lockfile generations); the next provision run touches only the current entry, X sorts oldest, and rm -rf deletes it out from under the in-flight worktree — every farm symlink dangles while node_modules/.package-lock.json still exists, so build-test keeps the repair install skipped and the review fails with module-not-found errors that read as defects in the PR. Latent preconditions: two review jobs concurrent on one host sharing $HOME, plus three lockfile generations inside one review's lifetime. Suggested fix: freshen the entry at consumption (utimesSync the validated entry in provisionWorktreeDependencies), or bound the prune with an age floor. Add a dep-provision assertion that the farmed entry's mtime is >= provision time; removing the freshen turns it red.
中文说明
[Suggestion] 剪枝的 LRU 信号(mtime)只为当前锁文件的条目刷新(warm 路径的 touch),但实际服务某评审的条目可能是更旧的——因此注释里"仍在服务评审的条目永不被驱逐"的说法,对服务于锁文件升级前 PR 的并发评审并不成立:fetch-pr 按 worktree 自己的锁文件选条目,从不触碰条目 mtime。一个长评审由条目 X 农场化;三个更新的条目出现(三次锁文件换代);下一次供给运行只 touch 当前条目,X 排序最旧,rm -rf 在评审运行中把它删掉——所有农场符号链接悬空,而 node_modules/.package-lock.json 仍在,build-test 继续跳过修复安装,评审以 module-not-found 失败,被读作 PR 的缺陷。潜在前提:同一宿主共享 $HOME 的两个并发评审 job,加一次评审生命周期内三次锁文件换代。建议修复:在消费端刷新条目(provisionWorktreeDependencies 里对被校验的条目 utimesSync),或给剪枝加年龄下限。新增 dep-provision 断言:被农场化条目的 mtime >= 供给时刻;删掉刷新逻辑测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R1-5] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R1-5] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
| echo 'no package-lock.json in the checkout; nothing to provision' | ||
| exit 0 | ||
| fi | ||
| HASH="$(sha256sum package-lock.json | cut -d' ' -f1)" |
There was a problem hiding this comment.
[Critical] The cache entry is keyed and reused on the lockfile hash ALONE — nothing records or checks the source revision the entry's dist trees were built from — so after any source-only base change (the common case: most merges don't touch the lockfile), every review whose worktree lockfile matches executes tests against sibling dist built from an OLDER commit than the tree under review, and no rebuild ever corrects it: build-test's rebuild scope is the closure of the PR-changed workspaces, so members the PR did not change keep the stale dist for the whole review. Executed end-to-end: run 1 cold-populated an entry with dist 'built at commit A'; run 2 after a source-only merge (lockfile unchanged) printed 'dependency cache warm' and left the entry untouched; run 3 provisioning a bare worktree at commit B returned provisioned: true, distCopied: 1 with the worktree holding dist 'built at commit A' beside src 'source at commit B'. A base-side regression introduced after the entry's commit then fails in probes and is published as evidence against an innocent PR — or a base-side fix masks a real PR failure. Pre-#10108 this could not happen: npm ci + prepare rebuilt everything from the worktree's own sources. Suggested fix: record the revision the entry was built from (git rev-parse HEAD into the stage, e.g. .qwen-review-source-rev, before the marker); the warm check requires it to equal the current checkout's HEAD, mismatch falls through to the cold path; and have provisionWorktreeDependencies decline (or skip the dist copies with a reason) unless the recorded rev matches the worktree's merge base. Add a warm-path scenario planting a rev-mismatched entry and asserting repopulation; removing the rev comparison turns it red.
中文说明
[Critical] 缓存条目只以锁文件哈希为键并被复用——没有任何东西记录或校验条目 dist 树构建自哪个源修订——于是任何纯源码的 base 变更之后(常态:多数合并不碰锁文件),每个 worktree 锁文件匹配的评审都在用比受审树更旧提交构建的兄弟 dist 跑测试,且任何重建都不会纠正:build-test 的重建范围是 PR 改动工作区的闭包,PR 未改的成员整场评审都保留陈旧 dist。已端到端执行:运行 1 冷填充条目,dist 为 'built at commit A';运行 2 在纯源码合并(锁文件未变)后打印 'dependency cache warm',条目原封不动;运行 3 对提交 B 的裸 worktree 供给,返回 provisioned: true, distCopied: 1,worktree 里 dist 是 'built at commit A' 而 src 是 'source at commit B'。条目提交之后引入的 base 侧回归会在探针里失败,被发布为对无辜 PR 的证据——或 base 侧修复掩盖真实的 PR 失败。#10108 之前不可能发生:npm ci + prepare 从 worktree 自己的源码重建一切。建议修复:记录条目构建自的修订(在标记前把 git rev-parse HEAD 写入 stage,如 .qwen-review-source-rev);warm 检查要求它与当前 checkout 的 HEAD 相等,不匹配则落入冷路径;并让 provisionWorktreeDependencies 在记录修订与 worktree 合并基不匹配时拒绝供给(或跳过 dist 复制并说明原因)。新增 warm 路径场景:植入修订不匹配的条目并断言重新填充;删掉修订比对即变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| tally.linked === 0 | ||
| ? 'the cache entry held nothing linkable' |
There was a problem hiding this comment.
[Suggestion] The suite never reaches the final guard's tally.linked === 0 arm ('the cache entry held nothing linkable'): no case builds a member-less worktree against an entry with nothing linkable, so the invariant "never claim provisioned when nothing was linked" has zero witnesses. A dependency-less repo is a real population output — npm ci still writes node_modules/.package-lock.json, so the entry passes the marker-handover check, and farmFromEntry skips that filename, linking nothing; a worktree without a workspaces field yields zero members. Probe-verified: the proposed case passes at HEAD (provisioned: false, reason 'the cache entry held nothing linkable', marker withheld); dropping || tally.linked === 0 flips it to provisioned: true with zero packages linked and the npm marker placed — build-test skips the repairing npm ci, and any module the probes import dies with ERR_MODULE_NOT_FOUND published as a defect in the diff — while all 16 existing tests stay green. Suggested fix: add a case with a worktree carrying only package-lock.json plus a package.json with no workspaces, and an entry whose node_modules holds only .package-lock.json; assert provisioned === false, the reason contains 'nothing linkable', and no marker handed over. Removing the guard turns it red.
中文说明
[Suggestion] 套件从未触达最终守卫的 tally.linked === 0 一臂('the cache entry held nothing linkable'):没有用例在无可链接条目前构建无成员 worktree,因此"什么都没链接就绝不声称已供给"的不变量零见证。无依赖仓库是真实的填充产物——npm ci 仍会写 node_modules/.package-lock.json,条目通过标记交接检查,而 farmFromEntry 跳过该文件名、什么都不链接;没有 workspaces 字段的 worktree 产生零成员。探针验证:建议用例在 HEAD 通过(provisioned: false,原因 'the cache entry held nothing linkable',扣住标记);删掉 || tally.linked === 0 会翻转为 provisioned: true、零包链接且放置 npm 标记——build-test 跳过修复用 npm ci,探针 import 的任何模块以 ERR_MODULE_NOT_FOUND 死掉并被发布为 diff 缺陷——而现有 16 个测试全绿。建议修复:加用例——worktree 只含 package-lock.json 加无 workspaces 的 package.json,条目 node_modules 只含 .package-lock.json;断言 provisioned === false、原因含 'nothing linkable'、未交出标记。删掉守卫即变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R1-24] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R1-24] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
| !farmFromEntry( | ||
| join(entryReal, 'node_modules'), | ||
| join(worktreeReal, 'node_modules'), |
There was a problem hiding this comment.
[Critical] The root farm mirrors the cache entry's .bin directory wholesale (the skip list omits .bin; the fixture counts it among the 6 linked entries), so workspace-member binaries resolve INSIDE the cache entry — to the base branch's member snapshot — bypassing the re-derived worktree self-links that the module header says exist precisely so probes never execute the base's copy. The divergence SURVIVES Agent 7's scoped rebuild, which rewrites the worktree's dist in place — the .bin resolution chain never traverses the worktree's re-pointed self-links. Verified against a real npm install (member bins link relative, e.g. .bin/corebin -> ../@fix/core/dist/cli.js) and probed at HEAD: with the worktree member's dist already rebuilt to PR content, realpath(wt/node_modules/.bin/corebin) lands in the ENTRY and reads 'built at base'. Trigger population: this repo has five bin-declaring members (qwen in packages/cli, node-repl, sdk-typescript, mobile-mcp, channels/plugin-example) — any PR touching one, warm cache, and a probe or agent running node_modules/.bin/qwen, npx qwen, or a member MCP server by name executes base bytes and publishes evidence about the base as a verdict about the PR, with the plan report asserting provisioned: true. Fires on the innocent steady-state path — no force-add, no manifest trick. Suggested fix: re-derive member bin links the way self-links are re-derived — after the farm and self-link passes, recreate node_modules/.bin/<binname> pointing at the WORKTREE member's bin target (or skip mirroring .bin and regenerate it from the entry's third-party bins plus the worktree members'). Extend makeEntry with a bin-declaring fixture member and assert realpathSync(wt/node_modules/.bin/corebin) lands under the worktree member — red at HEAD; the existing linked: 6 count needs its one-line adjustment with the fix.
中文说明
[Critical] 根农场整体镜像缓存条目的 .bin 目录(跳过清单不含 .bin;fixture 把它算进 6 个链接条目),于是 workspace 成员的二进制解析进缓存条目内部——解析到 base 分支的成员快照——绕过了模块头声明正是为了让探针永不执行 base 副本而重新推导的 worktree self-link。这一偏离在 Agent 7 的定向重建之后依然存在:重建就地改写 worktree 的 dist,而 .bin 的解析链从不经过 worktree 重新指向的 self-link。已对真实 npm install 验证(成员 bin 相对链接,如 .bin/corebin -> ../@fix/core/dist/cli.js),并在 HEAD 探针:worktree 成员 dist 已重建为 PR 内容时,realpath(wt/node_modules/.bin/corebin) 落在条目里,读出 'built at base'。触发面:本仓库有五个声明 bin 的成员(packages/cli 的 qwen、node-repl、sdk-typescript、mobile-mcp、channels/plugin-example)——任何触碰其一的 PR,warm 缓存下,探针或 agent 运行 node_modules/.bin/qwen、npx qwen 或按名字运行成员 MCP 服务器,都会执行 base 字节,把关于 base 的证据发布为关于 PR 的裁决,而 plan report 声称 provisioned: true。在无辜的稳态路径触发——无需 force-add、无需清单花招。建议修复:像重新推导 self-link 那样重新推导成员 bin 链接——在农场与 self-link 两遍之后,把 node_modules/.bin/<binname> 重建为指向 worktree 成员的 bin 目标(或跳过镜像 .bin,从条目的第三方 bin 加 worktree 成员的重新生成)。给 makeEntry 加一个声明 bin 的 fixture 成员,断言 realpathSync(wt/node_modules/.bin/corebin) 落在 worktree 成员下——HEAD 上是红的;修复时现有的 linked: 6 计数需一行调整。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| }, | ||
| }); | ||
| expect(r.status).toBe(0); | ||
| expect(r.stdout).toContain('dependency cache warm'); |
There was a problem hiding this comment.
[Suggestion] The warm-path scenario pins stdout, no-install, and workspace-untouched, but never pins the warm branch's mtime freshening (touch "$ENTRY" in the script) — the only mechanism keeping a still-serving entry newest for the LRU prune the suite's own prune scenario exercises — so deleting that line ships green (probe-verified: all 11 scenarios pass with the touch dropped). On the persistent pool an entry's mtime then stays frozen at its population instant while it keeps serving warm hits; once three newer lockfiles are populated, the prune evicts the still-serving entry, and every later review keyed on that lockfile silently degrades to the in-agent install #10108 exists to eliminate — nothing reported, suite green. Suggested fix: in prepare, backdate the prepared entry with the suite's existing mechanism (touch -d @1600000000), then assert the freshen — implementation note: the mtime must be captured inside the returned result object (e.g. an entryMtimeMs field), because runDepsCacheStep's finally deletes the fixture before the test body runs (statSync(r.entry) in the body hits ENOENT — verified). Removing touch "$ENTRY" turns the assertion red while every existing scenario stays green; lives inside R1-3's capability gate if that lands.
中文说明
[Suggestion] warm 路径场景钉住了 stdout、零安装、workspace 未动,却从不钉住 warm 分支的 mtime 刷新(脚本里的 touch "$ENTRY")——让仍在服务的条目在套件自己演练的 LRU 剪枝中保持最新的唯一机制——于是删掉该行绿灯上线(探针验证:去掉 touch 后 11 个场景全过)。持久池上条目 mtime 自此冻结在填充时刻,同时继续服务 warm 命中;三个更新的锁文件填充后,剪枝驱逐仍在服务的条目,之后每个以该锁文件为键的评审静默降级为 #10108 要消灭的 agent 内安装——无报告,套件绿。建议修复:在 prepare 里用套件现有机制(touch -d @1600000000)回拨预建条目的时间,然后断言刷新——实现注意:mtimе 必须在返回的结果对象里捕获(如 entryMtimeMs 字段),因为 runDepsCacheStep 的 finally 在测试体运行前删除 fixture(测试体里 statSync(r.entry) 会 ENOENT——已验证)。删掉 touch "$ENTRY" 使断言变红,现有场景保持绿色;若 R1-3 的门控落地则置于其中。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R1-26] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R1-26] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
| it('populates a cold cache: keyed by lockfile hash, marker present, root bundle excluded', () => { | ||
| const r = runDepsCacheStep(); | ||
| expect(r.status).toBe(0); | ||
| expect(r.calls).toContain('npm ci'); |
There was a problem hiding this comment.
[Suggestion] The cold-path scenario pins only the npm ci prefix of the logged invocation; prepare-hook inclusion — which the script's own comment calls the load-bearing half of the cache ("the hook builds every workspace's dist, which is exactly the prebuilt-sibling half of what the cache exists to serve") — is unpinned, so --ignore-scripts (or a QWEN_SKIP_PREPARE=1 on the ci, equally invisible to the stub) ships green and populates dist-less entries. Probe-verified: an npm ci --ignore-scripts mutant leaves all 11 scenarios green (the stub logs the full invocation, but toContain('npm ci') still matches). On the lane, npm then skips the prepare hook; the entry is snapshotted with no member dist anywhere; every review farmed from it passes build-test's completeness gate via the npm marker (install skipped), then probes and Agent 7 importing a workspace package fail with module-not-found, published as verdicts about the PR — and the entry serves that way until LRU-pruned. Suggested fix: pin the exact logged invocation — expect(r.calls).toContain('npm ci --no-audit --no-fund --progress=false'); — optionally plus expect(r.calls).not.toContain('--ignore-scripts') and that depsCacheSource() contains no QWEN_SKIP_PREPARE. Adding --ignore-scripts to the script turns the pinned assertion red.
中文说明
[Suggestion] 冷路径场景只钉住日志调用里的 npm ci 前缀;prepare 钩子的保留——脚本自己的注释称之为缓存的承重一半("钩子构建每个 workspace 的 dist,这正是缓存要服务的预构建兄弟那一半")——未被钉住,于是 --ignore-scripts(或在 ci 上加 QWEN_SKIP_PREPARE=1,对桩同样不可见)绿灯上线并填充无 dist 条目。探针验证:npm ci --ignore-scripts 变异让 11 个场景全绿(桩记录完整调用,但 toContain('npm ci') 仍匹配)。通道上 npm 随之跳过 prepare 钩子;条目快照里任何成员 dist 都没有;每个由它农场化的评审都经 npm 标记通过 build-test 完整性闸门(安装跳过),随后探针与 Agent 7 import workspace 包时以 module-not-found 失败,被发布为关于 PR 的裁决——条目就这样服务到被 LRU 剪掉。建议修复:钉住确切的日志调用——expect(r.calls).toContain('npm ci --no-audit --no-fund --progress=false');——可选再加 expect(r.calls).not.toContain('--ignore-scripts') 及 depsCacheSource() 不含 QWEN_SKIP_PREPARE。给脚本加 --ignore-scripts 会使钉住的断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R1-27] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R1-27] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
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 — did not converge within the reverse-audit round cap of 5.
Not reviewed: the executable-script lint — .github/scripts/provision-review-deps.sh: shellcheck is not installed.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未审查：the executable-script lint — .github/scripts/provision-review-deps.sh: shellcheck is not installed。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| fi | ||
| HASH="$(sha256sum package-lock.json | cut -d' ' -f1)" | ||
| ENTRY="$CACHE_ROOT/$HASH" | ||
| if [ -f "$ENTRY/.qwen-review-deps-complete" ]; then |
There was a problem hiding this comment.
[Critical] R1-1: still stands — no commit has touched this file since this finding was filed, and the mechanism stands at the reviewed commit. The cache entry is trusted on the presence of .qwen-review-deps-complete alone, but entries live on a path writable by the unsandboxed PR code this same job executes, and nothing validates entry content. A malicious PR — or any PR test that writes under node_modules/<pkg> — can trojanize the current entry, or pre-create the entry for the current lockfile hash entirely, keeping the marker, a byte-identical package-lock.json copy and node_modules/.package-lock.json: the only things the warm check here and dep-provision.ts validate. The warm-path touch keeps a planted entry newest for the LRU prune, so every later review of any PR with an unchanged lockfile on this runner link-farms the poisoned tree, records provisioned: true, build-test skips its install, and probes execute attacker-chosen bytes while reviewing unrelated PRs.
Witness (round 1, code unchanged since): a pre-planted trojanized entry was farmed and served (provisioned: true, failed: 0); the real script's warm path against it exited 0 with zero npm calls; a single writeFileSync through a worktree farm link changed the shared entry's file.
Suggested fix: make entries self-authenticating or immutable — write a path+sha256 manifest into the stage before the atomic rename and verify it on this warm path (rebuild, not exit, on mismatch) and in dep-provision.ts before farming, failing closed; and/or chmod -R a-w the entry after publish (restoring writability in the prune path before rm -rf). Add a workflow test asserting a manifest-mismatched entry is rebuilt instead of warm-exiting, and a dep-provision test asserting such an entry returns provisioned: false — remove the verification and confirm both go red.
中文说明
[Critical] R1-1:仍然成立——自该发现提出后没有任何提交修改过本文件,机制在受审提交上依旧成立。缓存条目仅凭 .qwen-review-deps-complete 标记的存在即被信任,但条目所在路径可被同一 job 执行的、未加沙箱的 PR 代码写入,且条目内容从不做任何校验。恶意 PR——或任何在 node_modules/<pkg> 下写文件的 PR 测试——都可以向当前条目植入木马,甚至直接为当前锁文件哈希预先创建整个条目:只需保留标记文件、一份逐字节一致的 package-lock.json 副本和 node_modules/.package-lock.json——这正是此处 warm 检查与 dep-provision.ts 校验的全部内容。warm 路径的 touch 还会让植入条目在 LRU 剪枝中保持最新,于是该 runner 上之后每个锁文件未变的 PR 评审都会链接农场化这棵被污染的树,记录 provisioned: true,build-test 跳过安装,探针在评审无关 PR 时执行攻击者选定的字节。
见证(第 1 轮,代码此后未变):预植入的木马条目被成功农场化并提供(provisioned: true, failed: 0);真实脚本的 warm 路径对其退出 0 且零次 npm 调用;通过 worktree 农场链接的一次 writeFileSync 改掉了共享条目内的文件。
建议修复:让条目自证或不可变——在原子重命名前向 stage 写入路径+sha256 清单,并在此 warm 路径(不匹配时重建而非退出)和 dep-provision.ts 农场化前校验,失败即回退;或在发布后 chmod -R a-w 条目(剪枝路径 rm -rf 前恢复可写)。补上两个测试:清单不匹配的条目会被重建而非 warm 退出、此类条目返回 provisioned: false——删掉校验逻辑,确认两个测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // The NAME is PR content too, and it becomes a path under node_modules: | ||
| // reject anything that would escape (`..`, absolute, empty segments) | ||
| // by resolving the joined path and requiring containment. | ||
| const linkPath = resolve(join(worktreeReal, 'node_modules'), name); |
There was a problem hiding this comment.
[Critical] R1-2: still stands — the file is unchanged since this finding was filed. The self-link loop's only check is endpoint containment; it performs rmSync/mkdirSync/symlinkSync on paths derived from PR-controlled member names without validating name shape. Four probe-confirmed entrances, each reporting provisioned: true with failed: 0 and handing over the npm marker: (1) a member named "." (or 'foo/..') collapses onto the farm root — the occupied branch's rmSync deletes the entire farm including the provenance marker, node_modules is re-created as a symlink to the member dir, and the npm marker is written through it; (2) a multi-segment name like "plain-pkg/x" traverses an entry-farm symlink — the mkdir/rm/symlink land INSIDE the shared host cache entry, persistently poisoning every later same-lockfile review; (3) a member named ".qwen-review-farm" deletes the farm's own provenance marker, so provisionSourceOf returns null and every scratch tree built for this review loses its dependencies; (4) a member named ".bin" (or a bare scope "@fix") replaces the farm's .bin link / scope directory, making every binary unreachable. Reachable because readWorkspacePackages admits any non-empty string name and the lockfile byte-compare passes (the PR never regenerates package-lock.json).
Witness (round 1, code unchanged since): all four entrances executed, each returned provisioned: true, failed: 0 and handed over the npm marker.
Suggested fix: validate the name's shape before any filesystem operation — reject names npm rejects (leading . or _), reject anything that is not exactly one segment or exactly two segments with the first starting @ and none empty/./.., and reserve the farm-owned paths (.bin, .package-lock.json, .vite, .vite-temp, the provision marker); count tally.failed++ and continue. Each partial fix (shape rule alone, reserved-path list alone) leaves at least one entrance open. Add one test per entrance (modeled on the existing ../evil case, lockfile unchanged): assert provisioned === false, failed > 0, and the farm/entry/marker/.bin intact — each is red at HEAD.
中文说明
[Critical] R1-2:仍然成立——自该发现提出后文件未变。self-link 循环的唯一检查是端点包含性;它对来自 PR 可控成员名的路径执行 rmSync/mkdirSync/symlinkSync,却从不校验名字形状。四个经探针确认的入口,全部报告 provisioned: true、failed: 0 并交出 npm 标记:(1) 成员名 "."(或 'foo/..')坍缩到农场根——占用分支的 rmSync 删掉整个农场(含 provenance 标记),node_modules 被重建为指向成员目录的符号链接,npm 标记透过它写入;(2) 多段名如 "plain-pkg/x" 穿过条目农场符号链接——mkdir/rm/symlink 落在共享宿主缓存条目内部,持久毒化之后每个同锁文件评审;(3) 成员名 ".qwen-review-farm" 删掉农场自己的 provenance 标记,provisionSourceOf 返回 null,为本评审构建的每个 scratch 树都失去依赖;(4) 成员名 ".bin"(或裸作用域 "@fix")替换农场的 .bin 链接/作用域目录,所有可执行文件不可达。可达原因:readWorkspacePackages 接受任何非空字符串名,且锁文件字节比对通过。
见证(第 1 轮,代码此后未变):四个入口全部执行成功,均返回 provisioned: true, failed: 0 并交出 npm 标记。
建议修复:在任何文件系统操作前校验名字形状——拒绝 npm 拒绝的名字(前导 . 或 _),拒绝不是恰好一段、或不是恰好两段且首段以 @ 开头且无空/./.. 段的名字,并保留农场自有路径(.bin、.package-lock.json、.vite、.vite-temp、provision 标记);计 tally.failed++ 并 continue。任何部分修复都至少留一个入口。请为每个入口加一个测试(仿照现有 ../evil 用例,锁文件不变):断言 provisioned === false、failed > 0、农场/条目/标记/.bin 完好——每个在 HEAD 都是红的。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| } | ||
| } | ||
|
|
||
| describe('dependency-cache step (real bash, stubbed npm)', () => { |
There was a problem hiding this comment.
[Critical] R1-3: still stands — the suite is unchanged since this finding was filed, and the merge from main left both the lane and the suite as they were. This real-bash suite drives a GNU-only script (sha256sum, mv -T, find -printf) and uses a GNU-only fixture (touch -d @epoch in the prune case) but carries no capability gate, while scripts/tests/vitest.config.ts excludes this file on win32 only — the macOS nightly lane (test_macos, merge_group/schedule/workflow_dispatch; its pull_request trigger is off, so this PR's own checks report it skipped) runs it and fails: sha256sum absent → HASH empty → no entry keyed; BSD mv lacks -T → the entry never lands; the prune fixture's touch -d throws uncaught. ci.yml says "Read a red nightly as a blocker; nothing else will report it" — this is the exact class of incident #9220, and it was already raised as a follow-up in this PR's triage comments.
Witness (round 1, code unchanged since): modeled-BSD execution of the real script reds the warm-path, cold-cache and prune assertions (sha256sum: command not found → cold path on an empty hash → npm invoked); the lane never runs the production step (Linux-pool-only), so gating the test costs nothing.
Suggested fix: gate this describe on the host toolchain using the file's existing capability-probe convention (hasGnuMktemp/hasJq): probe sha256sum/find/mv/touch and describe.skipIf(!hasGnuDepsToolchain). The portable wiring block stays ungated. Remove the gate on a non-GNU host and the three named tests red again.
中文说明
[Critical] R1-3:仍然成立——自该发现提出后套件未变,main 合入也未改动通道与套件。这个真实 bash 套件驱动 GNU 专有脚本(sha256sum、mv -T、find -printf),剪枝用例还用了 GNU 专有 fixture(touch -d @epoch),却没有任何能力门控;而 scripts/tests/vitest.config.ts 只在 win32 上排除本文件——macOS nightly 通道(test_macos,merge_group/schedule/workflow_dispatch 触发;pull_request 触发关闭,所以本 PR 自己的检查显示 skipped)会运行它并失败:sha256sum 缺失 → HASH 为空 → 条目无法键控;BSD mv 没有 -T → 条目无法落位;剪枝 fixture 的 touch -d 直接抛错。ci.yml 写明"红色 nightly 视为阻塞,没有其他东西会报告它"——这正是 #9220 事故的同类。
见证(第 1 轮,代码此后未变):BSD 模拟下执行真实脚本,warm-path、冷缓存、剪枝断言全部变红;该通道从不运行生产步骤(仅 Linux 池),门控测试零成本。
建议修复:用文件里现成的能力探测惯例(hasGnuMktemp/hasJq)按宿主工具链门控本 describe:探测 sha256sum/find/mv/touch,describe.skipIf(!hasGnuDepsToolchain)。可移植的 wiring 块保持不门控。在非 GNU 宿主上删掉门控,三个点名测试重新变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| * population step produces. A marker that fails any of it is ignored, which | ||
| * restores exactly the pre-provisioning containment. | ||
| */ | ||
| export function provisionSourceOf(dependencyRoot: string): string | null { |
There was a problem hiding this comment.
[Critical] R1-14: still stands — the file is unchanged since this finding was filed. provisionSourceOf validates only the SHAPE of the path the .qwen-review-farm marker names (absolute, resolvable, outside the dependency root, completeness marker, real node_modules dir) — never its LOCATION: it never requires the path to sit under the cache root process.env[DEPS_CACHE_ENV]. The marker is PR content exactly when the provisioner did not write it — the provisioner refuses over committed node_modules BEFORE writing the marker (which force-adding the marker necessarily means), and never runs when the env is unset — so a committed marker can name any attacker-shaped host directory, and the new provisionRoot containment exception then admits committed links resolving there as 'internal' with failed: 0 and nothing disclosed. Distinct from R1-1: the attacker never touches the canonical cache, and entry hardening does not constrain what the marker may name.
Witness (round 1 probe, code unchanged since): a committed marker naming $HOME/.qwen-review-stash (outside the configured cache root) plus a committed link payload -> stash/node_modules/payload → provisionSourceOf returned the stash, farm {linked: 1, failed: 0}, attacker payload mirrored into every scratch tree — probes execute attacker code from a durable host directory that survives worktree discard and cache pruning. Committing node_modules/.package-lock.json alongside also satisfies build-test's completeness gate.
Suggested fix: require the recorded path under the configured cache root — read process.env[DEPS_CACHE_ENV], return null when unset, reject unless inside the cache root's realpath (strict variant: require exactly cacheRoot/<sha256(worktree lockfile)>). Add the two tests named in the finding; removing the cache-root check turns them red.
中文说明
[Critical] R1-14:仍然成立——自该发现提出后文件未变。provisionSourceOf 只校验 .qwen-review-farm 标记所指路径的形状(绝对、可解析、在依赖根之外、带完整性标记、真实 node_modules 目录),从不校验其位置:从不要求路径位于缓存根 process.env[DEPS_CACHE_ENV] 之下。标记恰恰在供给器没有写它时才是 PR 内容——供给器在写标记之前就已因已提交 node_modules 而拒绝(force-add 标记必然意味着这一点),且环境变量未设时根本不运行——因此已提交的标记可以指向任何攻击者形状的宿主目录,而新的 provisionRoot 包含例外随后把解析到那里的已提交链接当作 'internal' 接纳,failed: 0,毫无披露。与 R1-1 不同:攻击者不碰规范缓存,条目加固也约束不了标记可以指向什么。
见证(第 1 轮探针,代码此后未变):已提交标记指向 $HOME/.qwen-review-stash(在配置的缓存根之外)加已提交链接 payload -> stash/node_modules/payload → provisionSourceOf 返回该 stash,农场 {linked: 1, failed: 0},攻击载荷被镜像进每个 scratch 树——探针从工作树丢弃与缓存剪枝都奈何不了的持久宿主目录执行攻击者代码。一并提交 node_modules/.package-lock.json 还能满足 build-test 的完整性闸门。
建议修复:要求记录路径位于配置的缓存根之下——读 process.env[DEPS_CACHE_ENV],未设返回 null,除非在缓存根 realpath 内否则拒绝(严格变体:要求恰好 cacheRoot/<sha256(worktree 锁文件)>)。补上发现中点名的两个测试;删掉缓存根检查即变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const hash = createHash('sha256').update(lockBytes).digest('hex'); | ||
| let entry: string; | ||
| try { | ||
| entry = realpathSync(join(cacheRoot, hash)); |
There was a problem hiding this comment.
[Critical] R1-16: still stands, escalated — round 1 filed this as a Suggestion; this round's probe escalates it to Critical. Entry selection and admission are keyed on the lockfile ALONE — the farm never checks that the worktree's package.json manifests still match that lockfile. provisioned: true (and npm's completeness marker) is returned for trees whose manifests demand packages the cache entry cannot satisfy — lockfile byte-equality does not capture workspace-membership or member dependency declarations, which npm's own sync check does. A PR that edits a member's manifest without regenerating the lockfile gets a farm that claims completeness while the tree cannot resolve; build-test then skips the npm ci that would have surfaced npm's precise EUSAGE diagnosis, and the missing module re-appears in the scoped build/test framed as a defect in the diff — the exact misattribution the module's marker discipline exists to prevent. Bounding fact, disclosed for weighing: the trigger is an already-broken PR (manifest/lockfile desync) — no good PR is failed and no broken PR approved; the harm is wrong diagnosis and review outcome depending on cache warmth.
Witness (probe this round): fixture whose member declares left-pad with byte-identical lockfile → {"provisioned":true,"linked":2,"failed":0,"selfLinked":1}, left-pad absent from the farmed tree, npm marker placed; npm ci on the same tree exits 1 — npm error Missing: left-pad@1.3.0 from lock file.
Suggested fix: before the final gate, verify each worktree graph member's demand is met by the entry — minimally, tally.failed++ when a member's manifest declares dependencies (incl. dev/optional/peer) that resolve neither in the entry's node_modules nor in that member's nested one; the class-closing variant is to include the workspace manifests (root workspaces + member manifests) in the cache-entry key so manifest divergence can never select an entry at all. Must keep the existing added-member-with-no-deps test green.
Fix witness: a new case in dep-provision.test.ts — entry built for a member whose manifest lacks a dep, worktree's same member declares that dep (same lockfile bytes) → expect provisioned: false and no node_modules/.package-lock.json; removing the guard must turn it red.
中文说明
[Critical] R1-16:仍然成立,本轮升级——第 1 轮以 Suggestion 提出;本轮探针将其升级为 Critical。条目选择与接纳只以锁文件为键——农场从不检查 worktree 的 package.json 清单是否仍与该锁文件匹配。对清单要求的包超出条目所能满足的树,会返回 provisioned: true(并交出 npm 完整性标记)——锁文件字节相等并不能捕获 workspace 成员关系或成员依赖声明,而 npm 自己的同步检查会捕获。一个修改成员清单却不重新生成锁文件的 PR,会得到一个声称完整、实际无法解析的农场;build-test 随后跳过本可给出 npm 精确 EUSAGE 诊断的 npm ci,缺失模块改在定向构建/测试里以"diff 中的缺陷"面貌出现——这正是模块标记纪律要防止的误归因。边界事实(供权衡):触发条件是已经坏掉的 PR(清单/锁文件失步)——不会挂掉好 PR,也不会放过坏 PR;危害是错误诊断,以及评审结果依赖缓存冷热。
见证(本轮探针):成员声明 left-pad 而锁文件字节不变的 fixture → {"provisioned":true,"linked":2,"failed":0,"selfLinked":1},农场树中没有 left-pad,npm 标记已放置;同一棵树上 npm ci 退出 1 —— npm error Missing: left-pad@1.3.0 from lock file。
建议修复:在最终闸门之前,校验每个 worktree 图成员的依赖需求都能被条目满足——最低限度:当成员清单声明的依赖(含 dev/optional/peer)既不在条目的 node_modules 也不在该成员的嵌套安装中解析时,计 tally.failed++;彻底闭环的变体是把 workspace 清单(根 workspaces + 成员清单)纳入缓存条目键,使清单偏离根本无法选中条目。需保持现有"新增无依赖成员"测试为绿。
修复见证:在 dep-provision.test.ts 新增用例——条目构建时成员清单缺某依赖,worktree 同名成员声明该依赖(锁文件字节相同)→ 期望 provisioned: false 且没有 node_modules/.package-lock.json;删掉守卫后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const entry = makeEntry(cache); | ||
| symlinkSync(outside, join(entry, 'node_modules', 'escapee'), 'dir'); |
There was a problem hiding this comment.
[Suggestion] The suite's escape containment coverage pins only the root-level branch of farmFromEntry; the parallel escape-counting arm inside @-scope directories is never exercised by any of the 16 tests — the fixture's @scope holds only a real directory (inner) and the @fix scope's symlinks all resolve inside the entry ('self' → skipped), so the scoped loop's if (v === 'escape') { tally.failed++; continue; } arm never executes. A refactor of farmFromEntry that merges or rewrites the scoped loop and drops (or mis-orders) that arm goes fully unobserved: a broken snapshot carrying an escaping symlink at node_modules/@scope/<name> would then be place()d into the worktree with failed still 0, npm's completeness marker handed over, and provisioned: true claimed while a worktree link points at an unvouched path outside the operator-provisioned entry — the exact harm the root-level escape test exists to prevent, relocating through the one branch that test does not cover. Witness (probe): unmodified code on a scoped-escapee fixture → {provisioned:false, failed:1}, escapee absent, no marker; arm-deleted mutant → {provisioned:true, failed:0}, escapee linked, marker placed — existing suite 16/16 green against the mutant. Fix: extend the fixture in this test with symlinkSync(outside, join(entry, 'node_modules', '@scope', 'escapee'), 'dir') and assert got.failed is 2, got.provisioned is false, existsSync(join(wt, 'node_modules', '@scope', 'escapee')) is false while @scope/inner still links. Deleting the scoped escape arm makes the new assertions red.
中文说明
[Suggestion] 套件的逃逸包含覆盖只钉住了 farmFromEntry 的根级分支;@ 作用域目录内部的并行逃逸计数臂在全部 16 个测试中从未被演练——fixture 的 @scope 只有真实目录(inner),@fix 作用域的符号链接都解析到条目内部('self' → 跳过),所以作用域循环的 if (v === 'escape') { tally.failed++; continue; } 臂从不执行。一次合并或重写作用域循环并丢弃(或错排)该臂的重构将完全不被察觉:携带 node_modules/@scope/<name> 逃逸符号链接的损坏快照会被 place() 进 worktree,failed 仍为 0,npm 完整性标记被交出,声称 provisioned: true,而 worktree 链接指向运营者供给条目之外一个未担保的路径——正是根级逃逸测试要防止的危害,搬到该测试未覆盖的唯一分支。见证(探针):未改代码在作用域逃逸 fixture 上 → {provisioned:false, failed:1},escapee 缺席、无标记;删臂突变体 → {provisioned:true, failed:0},escapee 被链接、标记已放置——现有套件对突变体 16/16 绿。修复:在本测试 fixture 中补 symlinkSync(outside, join(entry, 'node_modules', '@scope', 'escapee'), 'dir'),断言 got.failed 为 2、got.provisioned 为 false、existsSync(join(wt, 'node_modules', '@scope', 'escapee')) 为 false 而 @scope/inner 仍被链接。删掉作用域逃逸臂会让新断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R2-4] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R2-4] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
| entry.name === '.vite' || | ||
| entry.name === '.vite-temp' || |
There was a problem hiding this comment.
[Suggestion] The .vite/.vite-temp skip entries in farmFromEntry have zero test coverage — no fixture in dep-provision.test.ts creates either name (grep matches only the vitest import), so this guard ships unwitnessed; the sibling .package-lock.json skip is at least indirectly exercised by the handover assertions. If a later edit drops .vite/.vite-temp from the skip list (or reorders the guard), farmFromEntry's final place() mirrors a populated cache entry's vite build cache into every worktree's node_modules/.vite as a symlink — and the population step CAN snapshot it: it snapshots every node_modules dir after an npm ci whose prepare hook runs the web-shell's vite build, and vite's default dep cache is node_modules/.vite. Any build/dev run in a provisioned worktree then writes its cache through that link into the shared entry — the write-through class the dist-COPY design exists to prevent — while the suite stays green. Witness (probe): skip lines deleted in the scratch tree → the entire 16-test suite green; an entry fixture carrying node_modules/.vite → worktree .vite materialised: isSymlink=true resolvesTo=<entry>/node_modules/.vite. Fix: add a .vite directory (and .vite-temp) to the makeEntry fixture and assert in the happy-path test that existsSync(join(wt, 'node_modules', '.vite')) is false — removing either name from the skip list turns it red.
中文说明
[Suggestion] farmFromEntry 中的 .vite/.vite-temp 跳过项零测试覆盖——dep-provision.test.ts 没有任何 fixture 创建这两个名字(grep 只匹配到 vitest 导入),这个守卫在无见证下发布;兄弟 .package-lock.json 跳过至少被交接断言间接演练。若日后某次编辑把 .vite/.vite-temp 从跳过清单中去掉(或重排守卫),farmFromEntry 最后的 place() 会把已填充缓存条目的 vite 构建缓存以符号链接镜像进每个 worktree 的 node_modules/.vite——而填充步骤确实可能快照到它:它在 npm ci(其 prepare 钩子运行 web-shell 的 vite build)之后快照每个 node_modules 目录,vite 的默认依赖缓存正是 node_modules/.vite。已供给 worktree 里的任何 build/dev 运行都会穿过该链接把缓存写进共享条目——正是 dist 复制设计要防止的写穿一类——而套件保持绿。见证(探针):在 scratch 树删掉跳过行 → 整个 16 测试套件绿;携带 node_modules/.vite 的条目 fixture → worktree .vite materialised: isSymlink=true resolvesTo=<entry>/node_modules/.vite。修复:在 makeEntry fixture 里加 .vite 目录(和 .vite-temp),并在 happy-path 测试里断言 existsSync(join(wt, 'node_modules', '.vite')) 为 false——从跳过清单删除任一名字即变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R2-5] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R2-5] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
| expect( | ||
| lstatSync(join(wt, 'packages', 'core', 'dist')).isSymbolicLink(), | ||
| ).toBe(false); |
There was a problem hiding this comment.
[Suggestion] The 'dist is a COPY' pin asserts only not-a-symlink; it never verifies the write-isolation half of its own comment ('a rebuild overwrites the worktree, not the cache'), so any same-inode materialisation (hardlink) passes every assertion while defeating the property being pinned. Hardlinking is this feature's own idiom — the population script's cp -al. If the cpSync in dep-provision.ts is ever swapped for a recursive hardlink walk as a speed-up, the suite stays fully green — isSymbolicLink() is false and the content read matches — but Agent 7's scoped build then overwrites a member's dist in place through the hardlink, the write lands in the shared cache entry, and every subsequently farmed worktree is silently served the mutated base dist. Witness (probe): hardlink-walk mutant → inodes: worktree=8931073 entry=8931073 same=true; after an in-place overwrite of the worktree dist file, entry dist after worktree overwrite: built by PR — the shared entry corrupted; existing suite 16/16 green throughout. Fix: after provisionWorktreeDependencies returns, overwrite join(wt, 'packages', 'core', 'dist', 'index.js') with new content and assert the entry-side copy (join(entry, 'packages', 'core', 'dist', 'index.js')) still reads 'built at base\n' — or assert the two files' inodes differ. Swapping cpSync for a hardlinking copy turns that assertion red.
中文说明
[Suggestion] 'dist 是复制' 的钉住只断言了"不是符号链接";它从不验证自身注释的另一半('重建覆盖 worktree,而不是缓存'),于是任何同 inode 实体化(硬链接)都能通过全部断言,同时击穿被钉住的属性。硬链接正是本功能的惯用法——填充脚本的 cp -al。若 dep-provision.ts 的 cpSync 某天为提速换成递归硬链接遍历,套件全绿——isSymbolicLink() 为 false、内容读取匹配——但 Agent 7 的定向重建随后穿过硬链接就地覆盖成员 dist,写入落在共享缓存条目里,之后每个被农场化的 worktree 都静默拿到被改写的 base dist。见证(探针):硬链接遍历突变体 → inodes: worktree=8931073 entry=8931073 same=true;就地覆写 worktree dist 文件后,entry dist after worktree overwrite: built by PR——共享条目被破坏;整个过程现有套件 16/16 绿。修复:在 provisionWorktreeDependencies 返回后,用新内容覆写 join(wt, 'packages', 'core', 'dist', 'index.js'),断言条目一侧的副本(join(entry, 'packages', 'core', 'dist', 'index.js'))仍读作 'built at base\n'——或断言两个文件的 inode 不同。把 cpSync 换成硬链接复制会让该断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R2-6] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R2-6] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
| * the same guard `exposeDependencies` applies to the same untrusted input | ||
| * (the member list comes from the manifest of the code under review). | ||
| */ | ||
| function containedIn(root: string, dir: string): string | null { |
There was a problem hiding this comment.
[Suggestion] The escape guard for PR-controlled workspace-member paths now exists as two byte-identical copies — this one and containedIn in worktree.ts:1532 — instead of a shared implementation, and the two must be patched in lockstep forever. Both copies sanitize the same untrusted input (the workspace member list from the manifest under review) on write paths: this copy gates where provisionWorktreeDependencies creates nested farms (farmFromEntry into join(target, 'node_modules')), copies dist, and points self-link targets; the worktree.ts copy gates exposeDependencies' farm loop. This guard class has a documented history of bypass fixes in this very file family — worktree.ts's member-loop comment records measured escapes (workspaces: ["../.."], committed-symlink members). A future hardening that lands on only one copy — plausibly worktree.ts, where worktree.test.ts's escape suite pins behaviour — leaves the provisioning-side copy open to the same ../symlinked-member shapes, re-introducing on the provisioning path exactly the escape the scratch-tree path just fixed, with no test red. worktree.ts already imports from ./dep-provision.js, so sharing costs one export.
Witness: diff <(sed -n '559,567p' dep-provision.ts) <(sed -n '1532,1540p' worktree.ts) → empty (bodies byte-identical; only the JSDoc differs). The dep-provision copy is added by this diff; the worktree copy is pre-existing context.
Suggested fix: export containedIn from dep-provision.ts (or a small shared module) and import it in worktree.ts, deleting the duplicate. Both existing suites (dep-provision.test.ts member-farm cases, worktree.test.ts escape cases) already pin the behaviour against a single implementation.
中文说明
[Suggestion] 针对 PR 可控 workspace 成员路径的逃逸守卫生成了两份逐字节相同的副本——本文件这份与 worktree.ts:1532 的 containedIn——而不是共享实现,两者必须永远同步修补。两份副本都在写路径上净化同一份不可信输入(受审代码清单里的 workspace 成员列表):这份守卫决定 provisionWorktreeDependencies 在哪里创建嵌套农场(farmFromEntry 进入 join(target, 'node_modules'))、复制 dist、指向 self-link 目标;worktree.ts 那份守卫 exposeDependencies 的农场循环。这一守卫类在本文件家族里有记录的绕过修复史——worktree.ts 成员循环注释记录了实测逃逸(workspaces: ["../.."]、committed 符号链接成员)。未来某次加固只落在其中一份上——很可能是 worktree.ts,因为 worktree.test.ts 的逃逸套件钉住那里的行为——就会让供给一侧的副本对同样的 ../符号链接成员形状敞开,在供给路径上重新引入 scratch 树路径刚修掉的逃逸,且无测试变红。worktree.ts 已从 ./dep-provision.js 导入,共享只需一个 export。
见证:两份函数体逐字节相同(仅 JSDoc 不同);dep-provision 副本由本 diff 新增,worktree 副本是既有上下文。
建议修复:从 dep-provision.ts 导出 containedIn(或建小的共享模块),在 worktree.ts 导入并删除重复。两个现有套件(dep-provision.test.ts 成员农场用例、worktree.test.ts 逃逸用例)已对单一实现钉住行为。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R2-7] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R2-7] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
| const step = doc.jobs['review-pr'].steps.find( | ||
| (s) => s.name === 'Provision review dependency cache', | ||
| ); | ||
| expect(step.if).toBe( |
There was a problem hiding this comment.
[Suggestion] This test asserts step.if "mirrors the job's runs-on pool selector" but pins only the step's if: string — it never reads doc.jobs['review-pr']['runs-on'], so the two sides of the pool-selection/cache mechanism can drift with every test still green. Both sides agree at HEAD (both carry the github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true' pool terms — verified), but nothing pins that agreement. A future edit to runs-on alone (a pool migration to unconditional ubuntu-latest, or dropping the vars term from one side only) makes the provision step run on ephemeral hosted runners: every review pays a full cold npm ci plus the prepare-hook build — the ~161-second blocking prefix this feature exists to remove — to populate $HOME/.qwen-review-deps on a runner whose $HOME dies with the job, a cache that can never go warm; the inverse drift silently skips provisioning on the persistent pool. The suite stays green because this test compares a single string — exactly the "two sides never agree on identity" shape the cache mechanism cannot survive. Witness (probe): with runs-on mutated to 'ubuntu-latest' the wiring test still passes (1 passed); adding the pin below turns the same mutation red (AssertionError: expected 'ubuntu-latest' to contain 'ecs-qwen'); workflow restored, fixed test green.
| expect(step.if).toBe( | |
| expect(step.if).toBe( | |
| "steps.context.outputs.should_run == 'true' && github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true'", | |
| ); | |
| expect(JSON.stringify(doc.jobs['review-pr']['runs-on'])).toContain('ecs-qwen'); |
(Or pin the full runs-on expression literal / the vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true' term — any assertion that reds when pool selection decouples from the step gate.) Rewriting the job's runs-on to unconditional ubuntu-latest must turn it red; today it stays green under that mutation.
中文说明
[Suggestion] 该测试断言 step.if "镜像 job 的 runs-on 池选择器",却只钉住步骤的 if: 字符串——它从不读取 doc.jobs['review-pr']['runs-on'],于是池选择/缓存机制的两侧可以在所有测试保持绿色的情况下各自漂移。HEAD 上两侧一致(都携带 github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true' 池项——已验证),但没有任何东西钉住这种一致。未来单独修改 runs-on(把池迁移到无条件 ubuntu-latest,或只从一侧去掉 vars 项)会让供给步骤跑在临时托管 runner 上:每个评审都要付一次完整冷 npm ci 加 prepare 钩子构建——正是本功能要移除的约 161 秒阻塞前缀——去填充一个 $HOME 随 job 消亡的 runner 上的 $HOME/.qwen-review-deps,一个永远无法变热的缓存;反向漂移则会在持久池上静默跳过供给。套件保持绿,因为本测试只比较一个字符串——正是缓存机制无法承受的"两侧从不就身份达成一致"的形状。见证(探针):把 runs-on 突变为 'ubuntu-latest',接线测试仍通过(1 passed);加上下方钉住后,同样的突变变红(AssertionError: expected 'ubuntu-latest' to contain 'ecs-qwen');恢复 workflow,修复后的测试为绿。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
[R2-8] Deferred to the next round: this round ran under the budget brake (two prior rounds exhausted the agent budget before finishing), so it implemented only the blocking subset — the nine Critical findings, now fixed with witnessed tests in commit a630b7c. This Suggestion remains queued and is in scope for the next round.
中文说明
[R2-8] 推迟到下一轮：本轮在预算刹车下运行（此前两轮在完成任何工作前耗尽了预算），因此只实现了阻断子集——九个 Critical 发现，已在提交 a630b7c 中以有见证的测试修复。该 Suggestion 仍在队列中，属于下一轮范围。
|
🔄 AutoFix is working on this PR — round 5/100. Watch live progress; this round posts its report here when it finishes. 中文说明🔄 AutoFix 正在处理此 PR —— 第 5/100 轮。查看实时进度；本轮结束后会在此发布报告。 |
|
🤖 AutoFix ran out of time before finishing (timeout (2700000ms)) (attempt 1/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (2700000ms)）（第 1/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33059559161 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix ran out of time before finishing (timeout (3600000ms)) (attempt 2/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (3600000ms)）（第 2/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33075432245 🧠 Handled by Qwen Code · model/模型 |
# Conflicts: # packages/core/src/skills/bundled/review/SKILL.md
…und's Criticals Seal cache entries with a path+sha256 manifest and the source revision they were built from, and verify both before an entry is served (bash warm path rebuilds on mismatch; the provisioner fails closed). Make entry files immutable after publish so farm-link writes cannot poison later reviews. Validate workspace member names before any filesystem operation in the self-link loop, bound the farm marker's answer to the configured cache root, refuse entries whose recorded revision is not the worktree's merge base, count members whose manifest demand the entry cannot satisfy, re-derive the root `.bin` from the worktree's own members instead of mirroring the base's, and admit only node_modules-resolving links through the provisionRoot containment escape. Gate the GNU-only bash scenario suite on the host toolchain and realpath the provisioner test fixtures' temp roots.
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #10129 (address-review)Budget posture: the feedback carried a budget warning (2 prior rounds exhausted the time budget before finishing), so this round implemented the smallest blocking subset — all 9 Critical findings — and defers every Suggestion to the next round via Conflict resolution ( Critical findings — all addressed in code (commit a630b7c)
Suggestions — deferred to the next round (budget brake)All 28 still-open Suggestion findings (R1-4…R1-13, R1-15, R1-17…R1-19, R1-21, R1-22, R1-24, R1-26, R1-27, R2-2…R2-8) are deferred with per-thread replies in Pre-existing failure disclosed (NOT caused by this diff)
Verification
中文说明本轮摘要 — PR #10129（address-review）预算状态： 反馈携带预算警告（此前 2 轮都在完成任何工作前耗尽时间预算），因此本轮实现最小阻断子集——全部 9 个 Critical 发现——并通过 冲突解决（ Critical 发现——全部以代码解决（提交 a630b7c）
Suggestion——推迟到下一轮（预算刹车）全部 28 个未决 Suggestion 发现（R1-4…R1-13、R1-15、R1-17…R1-19、R1-21、R1-22、R1-24、R1-26、R1-27、R2-2…R2-8）均被推迟，每个线程的回复见 披露的既有失败（非本 diff 引入）
验证
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-5 prune's LRU mtime is only freshened for the current lockfile's entry — still stands; already reported in round 1 (open thread on provision-review-deps.sh)
- R1-7 cross-language KEEP-IN-SYNC literal pin binds only the shell side — still stands; already reported in round 1 (open thread on qwen-pr-review-workflow.test.js)
- R1-13 self-link loop's legitimate OCCUPIED branch has no coverage — still stands; already reported in round 1 (open thread on dep-provision.test.ts)
- R2-2 'falls back on a cold cache with the lockfile hash in the reason' never asserts the hash — still stands; already reported in round 2 (open thread on dep-provision.test.ts)
Not reviewed: reverse audit — rounds 1-3 each reported findings and the loop was stopped after round 3 of the 5-round cap so this round could complete and post its proven findings.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 2: executing dep-provision.test.ts to empirically confirm the suite passes (no node_modules in worktree; install+build exceeds budget); chunk 6: live execution of the three new tests in worktree.test.ts (review worktree has no node_modules ; full npm ci + prepare build not run in the shared tree).
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Convergence: round 3 posted 13 inline comment(s), 13 of them reported for the first time; the previous round posted 35 (8 new). Findings keep coming back to the same files: .github/scripts/provision-review-deps.sh (findings in rounds 1, 2; 4 more now); packages/cli/src/commands/review/lib/dep-provision.test.ts (findings in rounds 1, 2; 3 more now); packages/cli/src/commands/review/lib/dep-provision.ts (findings in rounds 1, 2; 3 more now), and 1 more file(s). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — rounds 1-3 each reported findings and the loop was stopped after round 3 of the 5-round cap so this round could complete and post its proven findings。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 2：executing dep-provision.test.ts to empirically confirm the suite passes (no node_modules in worktree; install+build exceeds budget)；chunk 6：live execution of the three new tests in worktree.test.ts (review worktree has no node_modules ; full npm ci + prepare build not run in the shared tree)。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛情况：第 3 轮发布了 13 条行内评论，其中 13 条是首次提出；上一轮发布了 35 条（其中 8 条首次提出）。发现反复回到同一批文件：.github/scripts/provision-review-deps.sh（第 1、2 轮已出过发现，本轮又有 4 条）；packages/cli/src/commands/review/lib/dep-provision.test.ts（第 1、2 轮已出过发现，本轮又有 3 条）；packages/cli/src/commands/review/lib/dep-provision.ts（第 1、2 轮已出过发现，本轮又有 3 条），另有 1 个文件。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| # Immutable files after publish: a write through a farm link by the PR | ||
| # code this job is about to execute dies EACCES instead of corrupting | ||
| # every later review served from this entry. Directories stay writable, | ||
| # so the prune and any teardown can still unlink. | ||
| find "$ENTRY" -type f -exec chmod a-w {} + 2>/dev/null |
There was a problem hiding this comment.
[Critical] R1-1: (fix-induced) The round-2 fix closed the entrance round 1 reported — in-place tamper of a sealed file now fails the manifest hash check — but the seal it added is self-referential and forgeable, so cross-review cache poisoning stays reachable. All three trust anchors (.qwen-review-source-rev, .qwen-review-deps-manifest, .qwen-review-deps-complete) live INSIDE the entry they vouch for; entry directories deliberately stay writable; chmod a-w blocks in-place writes but not unlink-and-recreate. The same unsandboxed PR user whose code this job executes can therefore (a) swap or add symlinks — skipped by the manifest, the file count, entry_verifies, and the TS entryManifestValid/walkFiles, and the root .bin rebuild below copies a tampered link text VERBATIM into the next worktree with no containment verdict — (b) unlink a sealed file, rewrite it, and regenerate a self-consistent manifest, or (c) replace the entry wholesale (payload node_modules, byte-identical lockfile, .qwen-review-source-rev = current main HEAD — all public information — self-authored manifest, marker); the shell warm check and the TS provisioner then both serve it. Additionally provisionWorktreeDependencies resolves entry = realpathSync(join(cacheRoot, hash)) without re-checking the result stays under the cache root (unlike provisionSourceOf), so a symlink planted AT the entry name resolving outside the cache is served too. Content checks over attacker-authored state can never close this class; the fix is provenance/trust separation.
Witness (probe, unmodified PR code): LEMMA: in-place write blocked=true; unlink=true; recreate=true; content="PAYLOAD"; CHANNEL1 (unlink+rewrite+regenerated manifest): provisioned=true failed=0, worktree resolves plain-pkg="PAYLOAD-A"; CHANNEL1b (wholesale replacement): provisioned=true failed=0 served; real script warm path: exit=0, dependency cache warm: …, npm called: 0, entry still carries payload; CHANNEL2: a symlink planted at the entry name was farmed (source outside the cache root) and applying the insideDir re-check flipped it to provisioned=false.
Suggested fix: separate the trust domains — root-own published entries (the workflow already uses sudo -n for ownership repair on this pool) and route prune/teardown rm -rf through sudo with warn-and-degrade, or store the manifest/source-rev outside the writable entry path and fail any entry whose external record is missing or disagrees; defense-in-depth: after realpathSync(join(cacheRoot, hash)) require insideDir(realpathSync(cacheRoot), entry). Until then, also seal the symlink set (record every symlink + target text; fail verification unless they match) and give the root .bin rebuild the same containment verdict every other mirrored link gets. Fix witness: in the workflow suite, after a publish, attempt as the non-root user to unlink a sealed entry file and regenerate the manifest, expecting entry_verifies to reject; in dep-provision.test.ts, replace an entry wholesale (resealed to match) expecting provisioned: false — remove the ownership/anchor relocation and both must go red.
中文说明
[Critical] R1-1：（修复引入）第 2 轮的修复关闭了第 1 轮报告的入口——对密封文件的原位篡改现在会被清单哈希检查拦截——但它新增的封印是自指且可伪造的，跨评审缓存投毒仍然可达。三个信任锚（.qwen-review-source-rev、.qwen-review-deps-manifest、.qwen-review-deps-complete）都位于它们所担保的条目内部；条目目录按设计保持可写；chmod a-w 只阻止原位写入，不阻止先解除链接再重建。本 job 即将执行的、未加沙箱的 PR 代码以同一 runner 用户运行，因此可以：(a) 替换或新增符号链接——清单、文件计数、entry_verifies 与 TS 侧 entryManifestValid/walkFiles 全部跳过符号链接，而下方的根 .bin 重建会把被篡改的链接文本逐字复制进下一个 worktree，且没有任何容纳判定；(b) 解除某个密封文件的链接、重写它并重新生成自洽的清单；或 (c) 整体替换条目（载荷 node_modules、逐字节一致的锁文件、.qwen-review-source-rev = 当前 main HEAD——均为公开信息——自写清单、标记文件），shell 热路径检查与 TS 供给器随后都会将其供给出去。此外 provisionWorktreeDependencies 解析 entry = realpathSync(join(cacheRoot, hash)) 后从不复查结果是否仍在缓存根之下（与 provisionSourceOf 不同），因此在条目名上植入一个解析到缓存之外的符号链接也会被供给。对攻击者可写的状态做内容校验永远无法关闭这一类问题；修复方向是来源证明/信任域分离。
见证（探针，未改动的 PR 代码）：LEMMA: in-place write blocked=true; unlink=true; recreate=true; content="PAYLOAD"；CHANNEL1（解除链接+重写+重生成清单）：provisioned=true failed=0，worktree 解析出 plain-pkg="PAYLOAD-A"；CHANNEL1b（整体替换）：provisioned=true failed=0 被供给；真实脚本热路径：exit=0、dependency cache warm: …、npm called: 0，条目仍携带载荷；CHANNEL2：植入条目名的符号链接被农场化（source 在缓存根之外），加上 insideDir 复查后翻转为 provisioned=false。
建议修复：分离信任域——将已发布条目归 root 所有（本工作流在该池上已用 sudo -n 做所有权修复），剪枝/清理的 rm -rf 经由 sudo 执行并在失败时告警降级；或将清单/源修订存放在可写条目路径之外，外部记录缺失或不一致即失败；纵深防御：realpathSync(join(cacheRoot, hash)) 之后要求 insideDir(realpathSync(cacheRoot), entry)。在此之前，还应把符号链接集合纳入封印（记录每个符号链接及其目标文本，不一致即校验失败），并让根 .bin 重建获得与其他镜像链接相同的容纳判定。修复见证：workflow 套件中发布后以非 root 用户尝试解除密封文件链接并重生成清单，断言 entry_verifies 拒绝；dep-provision.test.ts 中整体替换条目（重新密封至自洽）断言 provisioned: false——移除所有权/锚点外移后两者必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| for (const dep of demanded) { | ||
| const inEntry = existsSync( | ||
| join(entryReal, 'node_modules', ...dep.split('/')), | ||
| ); |
There was a problem hiding this comment.
[Critical] R1-16: (fix-induced) The demand check the round-2 fix added re-implements npm ci's manifest/lockfile sync check as a package-NAME existence check only — the declared version ranges (already parsed into each member manifest) are never read, and the root package.json is never consulted at all (the loop iterates workspace members only; in a repo without a workspaces field it never runs). A manifest/lockfile desync therefore passes the farm, which then places npm's .package-lock.json completeness marker — the exact gate build-test's installComplete() reads to skip npm ci — so the desync is detected nowhere and the very npm ci that would have rejected it loudly never runs. The comment above states the intent this misses: npm's sync check is range satisfaction, not name presence.
Witness (probe, unmodified PR code): member manifest bumped to ^2.0.0 with the lockfile unchanged → provisioned=true failed=0 linked=2, npm completeness marker placed, worktree resolves some-dep@1.0.0 while the manifest demands ^2.0.0. Oracle — real npm ci on the same desync shape: npm error Invalid: lock file's is-odd@1.0.0 does not satisfy is-odd@3.0.1 (exit 1). Candidate-fix tree (parse lockBytes + range satisfaction, fail closed): provisioned=false failed=1, no marker.
Suggested fix: for each demanded dep, additionally verify range satisfaction against the lockfile the farm already holds (read packages['node_modules/' + dep].version from lockBytes, count the member failed unless the declared range accepts it, fail closed when unverifiable), and include the root package.json in the demand pass with the same optional-exclusion treatment. Fix witness: an entry installing plain-pkg@1.0.0 with a member demanding plain-pkg: ^2.0.0 must return provisioned: false, failed > 0, plus a root-manifest variant (root declares a dep the entry lacks, including a no-workspaces repo); remove the range/root check and both go red while today's suite stays green.
中文说明
[Critical] R1-16：（修复引入）第 2 轮修复新增的需求检查把 npm ci 的清单/锁文件同步检查重新实现为仅按包名存在性检查——声明的版本范围（已解析进每个成员清单）从不被读取，根 package.json 也完全不被查询（该循环只遍历 workspace 成员；在没有 workspaces 字段的仓库中根本不会运行）。于是清单/锁文件失配能通过农场，农场随后放置 npm 的 .package-lock.json 完整性标记——恰是 build-test 的 installComplete() 用来跳过 npm ci 的闸门——失配在任何地方都不会被检出，而本会大声拒绝它的 npm ci 永远不会运行。上方注释声明的意图恰恰被遗漏：npm 的同步检查是范围满足，而非名称存在。
见证（探针，未改动的 PR 代码）：成员清单升到 ^2.0.0 而锁文件未变 → provisioned=true failed=0 linked=2，npm 完整性标记被放置，worktree 解析到 some-dep@1.0.0 而清单要求 ^2.0.0。神谕——真实 npm ci 对同一失配形态：npm error Invalid: lock file's is-odd@1.0.0 does not satisfy is-odd@3.0.1（退出码 1）。候选修复树（解析 lockBytes + 范围满足、失败即回退）：provisioned=false failed=1，无标记。
建议修复：对每个被需求的依赖，额外用农场已持有的锁文件校验范围满足（从 lockBytes 读取 packages['node_modules/' + dep].version，声明范围不接受即计入该成员失败，无法校验时失败即回退），并把根 package.json 纳入需求检查，采用相同的可选依赖排除处理。修复见证：条目安装 plain-pkg@1.0.0 而成员要求 plain-pkg: ^2.0.0 时必须返回 provisioned: false, failed > 0，另加根清单变体（根声明了条目缺失的依赖，含无 workspaces 字段的仓库）；移除范围/根检查后两者变红，而当前套件保持绿。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| for (const { linkName, target } of memberBins) { | ||
| try { | ||
| rmSync(join(binDir, linkName), { force: true }); | ||
| symlinkSync(relative(binDir, target), join(binDir, linkName)); |
There was a problem hiding this comment.
[Critical] R1-25: (fix-induced) The .bin re-derivation loop the round-2 fix added (replacing the wholesale .bin mirroring round 1 reported) uses member bin link names — the keys of a member manifest's bin object — as path components for rmSync/symlinkSync with no containment. The self-link loop above prevents exactly this class with isValidPackageName + insideDir (and the suite pins it for names like '../evil'), but this loop applies neither check to linkName, and the bin's resolved target is likewise uncontained.
Witness (probe, unmodified PR code): a member with "bin": { "../../../f32-planted": "./cli.js", "../../../f32-victim.txt": "./cli.js" } and no dependencies → provisioned: true failed: 0; f32-planted created as a symlink OUTSIDE the worktree (lstatSync(...).isSymbolicLink() === true); the pre-existing host file f32-victim.txt deleted and replaced by a symlink (victim now symlink: true, victim content gone: true).
Suggested fix: validate each member bin link name before use, in the same shape as the self-link path — require a single safe segment (non-empty, no leading ./_, no /), or resolve join(binDir, linkName) and require insideDir(binDir, …) before the rmSync/symlinkSync; count the member failed and skip otherwise, and contain the bin's resolved target the same way. Fix witness: a member with "bin": { "../evil": "./cli.js" } and no dependencies must be counted failed and create/delete nothing outside .bin (e.g. existsSync(join(wt, 'evil')) is false); remove the guard and that test goes red.
中文说明
[Critical] R1-25：（修复引入）第 2 轮修复新增的 .bin 重新推导循环（替代第 1 轮报告的 .bin 整体镜像）把成员 bin 链接名——成员清单 bin 对象的键——直接用作 rmSync/symlinkSync 的路径组件，没有任何容纳检查。上方的 self-link 循环用 isValidPackageName + insideDir 防住了完全相同的这一类（套件也为 '../evil' 等名字钉死了行为），但本循环对 linkName 两个检查都没用，且 bin 的解析目标同样未受约束。
见证（探针，未改动的 PR 代码）：某成员 "bin": { "../../../f32-planted": "./cli.js", "../../../f32-victim.txt": "./cli.js" } 且无依赖 → provisioned: true failed: 0；f32-planted 被创建为 worktree 之外的符号链接（lstatSync(...).isSymbolicLink() === true）；预先存在的宿主文件 f32-victim.txt 被删除并替换为符号链接（victim now symlink: true, victim content gone: true）。
建议修复：在使用前校验每个成员 bin 链接名，形态与 self-link 路径一致——要求单一安全段（非空、不以 ./_ 开头、不含 /），或在 rmSync/symlinkSync 前解析 join(binDir, linkName) 并要求 insideDir(binDir, …)；否则计入该成员失败并跳过，同时对 bin 的解析目标做相同约束。修复见证：成员 "bin": { "../evil": "./cli.js" } 且无依赖时必须计入失败，且 .bin 之外无任何创建/删除（如 existsSync(join(wt, 'evil')) 为 false）；移除守卫后该测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| cpSync(join(source, 'dist'), join(target, 'dist'), { | ||
| recursive: true, | ||
| }); | ||
| tally.distCopied++; |
There was a problem hiding this comment.
[Critical] R3-1: The round-2 hardening publishes cache-entry files read-only (chmod a-w in provision-review-deps.sh), cpSync preserves that 0444 mode into the worktree's copied prebuilt dist, and the eleven packages/channels/* workspaces build with bare tsc --build, which does NOT clear dist first — so the scoped rebuild Agent 7 always performs overwrites existing 0444 outputs in place and fails EACCES. This is a cache-only false build failure (the cold path installs fresh, writable files) on exactly the PRs this feature exists to serve; workspaces built via scripts/build_package.js (core, cli) are unaffected because it rmSync('dist') first.
Witness (probe): copied dist file mode 444 (dir 755); writeFileSync on the copy → EACCES (uid 1000); real tsc against a pre-existing 0444 output: error TS5033: Could not write file '/tmp/f33-tsc/dist/index.js': EACCES: permission denied, exit 2.
Suggested fix: restore owner-write on the copied tree after the copy — walk join(target, 'dist') after cpSync and chmodSync(f, statSync(f).mode | 0o200) for each file, keeping the shared entry immutable while the worktree copy is buildable.
| cpSync(join(source, 'dist'), join(target, 'dist'), { | |
| recursive: true, | |
| }); | |
| tally.distCopied++; | |
| cpSync(join(source, 'dist'), join(target, 'dist'), { | |
| recursive: true, | |
| }); | |
| for (const f of walkDistFiles(join(target, 'dist'))) { | |
| chmodSync(f, statSync(f).mode | 0o200); | |
| } | |
| tally.distCopied++; |
Fix witness: makeEntry should chmod entry files read-only (mirroring the population step), and 'farms a matching entry' should additionally assert writeFileSync on a copied dist file succeeds; remove the chmod-restore and that assertion goes red — today the whole interaction ships green.
中文说明
[Critical] R3-1：第 2 轮加固把缓存条目文件发布为只读（provision-review-deps.sh 中的 chmod a-w），cpSync 会把该 0444 模式保留进 worktree 复制来的预构建 dist，而 11 个 packages/channels/* 工作区用裸 tsc --build 构建、不会先清空 dist——于是 Agent 7 必然执行的定向重建会原位覆写 0444 产物并失败于 EACCES。这是仅存在于缓存路径的假构建失败（冷路径安装的是全新可写文件），恰好发生在该特性要服务的 PR 上；经 scripts/build_package.js 构建的工作区（core、cli）不受影响，因为它先 rmSync('dist')。
见证（探针）：复制出的 dist 文件模式 444（目录 755）；对副本 writeFileSync → EACCES（uid 1000）；真实 tsc 面对已存在的 0444 产物：error TS5033: Could not write file '/tmp/f33-tsc/dist/index.js': EACCES: permission denied，退出码 2。
建议修复：复制完成后恢复副本树的所有者写权限——cpSync 之后遍历 join(target, 'dist')，对每个文件 chmodSync(f, statSync(f).mode | 0o200)，共享条目保持不可变而 worktree 副本可构建。修复见证：makeEntry 应把条目文件 chmod 为只读（与填充步骤一致），'farms a matching entry' 额外断言对复制出的 dist 文件 writeFileSync 成功；移除 chmod 恢复后该断言变红——当前整个交互以绿通过。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| it('is null for an unprovisioned tree', () => { | ||
| expect(provisionSourceOf(makeWorktree())).toBe(null); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R3-2: Four provisionSourceOf tests — 'is null for an unprovisioned tree' (:651), 'rejects a marker naming a path inside the dependency root' (:655), 'rejects a marker naming a directory without the completeness marker' (:669) and 'rejects a relative or empty recorded path' (:678) — never set DEPS_CACHE_ENV, so they all return at the function's first-line env gate (const cacheRoot = process.env[DEPS_CACHE_ENV]; if (!cacheRoot) return null;) and pin none of the guards they were written for. The completeness-marker test is doubly unreachable: even with the env set, its plain dir lives outside any configured cache root, so cache-root containment rejects first. The sibling tests at :613/:639 correctly use withDepsCacheEnv.
Mutation-proven: deleting any of the guarded lines in turn — the dependency-root containment (:602), the cache-root containment (:603), the completeness-marker check (:604), the isAbsolute gate (:593), or the marker-read try/catch — leaves all 7 provisionSourceOf tests green, so the hardening guards added in the round-2 commit ship with no witness and can silently regress in any later refactor; the regression then surfaces as a scratch-tree farm throw in exposeDependencies, not in this file.
Suggested fix: wrap all four in withDepsCacheEnv(cache, () => …) with a fresh cache root; in the completeness-marker test create plain INSIDE that root so the marker check — not cache-root containment — is the rejecting branch; in the unprovisioned-tree test set the env with a valid entry present but no marker written, so the null answer comes from the marker-read branch alone. Fix witness: the rewired tests themselves — removing the respective guard line from provisionSourceOf must turn each red; today none of them can.
中文说明
[Suggestion] R3-2：四个 provisionSourceOf 测试——'is null for an unprovisioned tree'（:651）、'rejects a marker naming a path inside the dependency root'（:655）、'rejects a marker naming a directory without the completeness marker'（:669）、'rejects a relative or empty recorded path'（:678）——都未设置 DEPS_CACHE_ENV，因此全部在函数首行环境闸门（const cacheRoot = process.env[DEPS_CACHE_ENV]; if (!cacheRoot) return null;）处返回，没有钉住任何一个它们为之而写的守卫。完整性标记测试双重不可达：即便设置了环境变量，其 plain 目录也在任何已配置缓存根之外，会先被缓存根容纳检查拒绝。同块的 :613/:639 两个兄弟测试正确使用了 withDepsCacheEnv。
已用变异证实：依次删除任一被守卫的行——依赖根容纳（:602）、缓存根容纳（:603）、完整性标记检查（:604）、isAbsolute 闸门（:593）、标记读取 try/catch——全部 7 个 provisionSourceOf 测试都保持绿，因此第 2 轮提交新增的加固守卫没有任何见证，任何后续重构都可能让它们悄悄回归；回归会以 exposeDependencies 中 scratch-tree 农场抛错的形式出现，而不是在本文件中。
建议修复：把四个测试都包进 withDepsCacheEnv(cache, () => …) 并使用新的缓存根；完整性标记测试把 plain 建在该根内部，使拒绝分支落在标记检查而非缓存根容纳上；未供给树测试在环境已设、存在有效条目但未写标记的情形下断言，使 null 答案只来自标记读取分支。修复见证：重接后的测试本身——从 provisionSourceOf 移除对应守卫生效行后各自必须变红；如今任何一个都做不到。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
Deferred to the next round (batch cap). Verified real: the four provisionSourceOf tests at :651/:655/:669/:678 all return at the first-line env gate because they never set DEPS_CACHE_ENV, and the completeness-marker case is additionally unreachable through cache-root containment — deleting any of the guarded lines keeps all seven tests green, confirmed by inspection. The rewiring (wrap in withDepsCacheEnv, move the plain fixture inside the configured root, seed a valid marker-less entry for the unprovisioned case) is mechanical but test-line-additive; this round spent its growth budget on the four Criticals plus the cheapest witnesses. Carrying it forward with the exact suggested rewiring.
中文说明
顺延至下一轮（本轮批次上限）。已核实属实：:651/:655/:669/:678 四个 provisionSourceOf 测试都未设置 DEPS_CACHE_ENV，全部在函数首行环境闸门处返回；完整性标记用例还因缓存根容纳检查而双重不可达——删除任一被守卫的行，七个测试仍全部为绿，已核对确认。重接方案（包进 withDepsCacheEnv、把 plain fixture 移进所配置缓存根内部、为未供给用例种入一个有效但无标记的条目）是机械改动，但会增加测试行数；本轮的增长预算已用于四条 Critical 及最便宜的见证测试。按所建议的重接方案原样带到下一轮。
| expect(provision).toBeGreaterThan( | ||
| workflow.indexOf("- name: 'Resolve PR context'"), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-6: This ordering assertion is soft: when its anchor string is absent from the YAML, indexOf returns -1 and toBeGreaterThan(-1) passes trivially — pinning nothing exactly when the anchor step is renamed or removed — unlike the sibling 'Run review' assertion below, which fails loudly (toBeLessThan(-1) is unsatisfiable once provision > -1). The pre-existing capture-tools ordering test has the same shape, but these lines are added by this diff.
Runtime-verified: with - name: 'Resolve PR context' renamed and the provision step moved above it, all three assertions PASS (resolveContext idx=-1, provision > -1 trivially true), while the pinned if: (steps.context.outputs.should_run == 'true') reads an output that does not exist yet and the step silently skips on every run — the host dependency cache is never populated and issue #10108's budget failure silently returns — while this test stays green.
| expect(provision).toBeGreaterThan( | |
| workflow.indexOf("- name: 'Resolve PR context'"), | |
| ); | |
| const resolveContext = workflow.indexOf("- name: 'Resolve PR context'"); | |
| expect(resolveContext).toBeGreaterThan(-1); | |
| expect(provision).toBeGreaterThan(resolveContext); |
Fix witness: the anchor-existence assertion — rename or delete the - name: 'Resolve PR context' line in qwen-code-pr-review.yml and it goes red; today the test stays green.
中文说明
[Suggestion] R3-6：该顺序断言是软性的：当其锚字符串不在 YAML 中时，indexOf 返回 -1，toBeGreaterThan(-1) 平凡通过——恰在锚步骤被改名或删除时什么都不钉——与下方 'Run review' 断言不同，后者会大声失败（provision > -1 成立后 toBeLessThan(-1) 不可满足）。既有的 capture-tools 顺序测试形态相同，但本 diff 新增的是这几行。
已在运行时验证：把 - name: 'Resolve PR context' 改名并把 provision 步骤移到其上方后，三个断言全部通过（resolveContext idx=-1，provision > -1 平凡为真），而被钉住的 if:（steps.context.outputs.should_run == 'true'）读到的是尚不存在的输出，步骤在每次运行中都静默跳过——宿主依赖缓存永不填充，issue #10108 的预算失败悄然回归——而本测试保持绿。
修复见证：锚存在断言——把 qwen-code-pr-review.yml 中 - name: 'Resolve PR context' 一行改名或删除后它变红；如今该测试保持绿。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| echo '::warning::dependency-cache snapshot failed; discarding the stage' | ||
| rm -rf "$STAGE" | ||
| exit 0 |
There was a problem hiding this comment.
[Suggestion] R3-7: Three failure paths after a successful npm ci — snapshot failure (:114-116), seal failure (:132-135), and the manifest mv failure (:137) — exit 0 before the workspace cleanup at the end of the script, leaving the ~2GB node_modules/dist tree in the persistent runner's checkout: the exact state that cleanup exists to remove, per its own comment ('drop it from the workspace so the next job's checkout clean and ownership repair do not crawl a two-gigabyte tree'). The mv -T publish-failure else branch DOES fall through to the cleanup; only the earlier exits skip it.
Concrete cost: npm ci succeeds, then the snapshot or seal fails (a cp -al fault, mktemp failure, transient ENOSPC during the stage copy); the script exits with the built tree still in the workspace and no cache entry published, and the next review job on that runner pays chown -R + chmod -R u+rwX over the leftover tree in 'Restore workspace ownership' and a git clean crawl during checkout — for a tree that no longer serves anything.
Suggested fix: restructure so every path after a successful npm ci reaches the two cleanup find … rm -rf passes — convert the snapshot/seal early exits into ok=false fall-through that runs cleanup before exit 0, or duplicate the cleanup block on those exits. Fix witness: a workflow-suite scenario that forces a snapshot failure after a stubbed successful npm ci and asserts node_modules/dist are gone from the checkout; nothing pins post-failure workspace state today.
中文说明
[Suggestion] R3-7：npm ci 成功之后的三条失败路径——快照失败（:114-116）、密封失败（:132-135）、清单 mv 失败（:137）——都在脚本末尾的 workspace 清理之前 exit 0，把约 2GB 的 node_modules/dist 树留在持久 runner 的 checkout 中：恰是清理逻辑按自身注释要移除的状态（'drop it from the workspace so the next job's checkout clean and ownership repair do not crawl a two-gigabyte tree'）。mv -T 发布失败的 else 分支会落入清理；只有更早的出口跳过了它。
具体代价：npm ci 成功后快照或密封失败（cp -al 故障、mktemp 失败、阶段复制时瞬时 ENOSPC）；脚本带着已构建的树仍在 workspace 且未发布任何缓存条目退出，该 runner 上的下一个评审 job 要在 'Restore workspace ownership' 中对残留树执行 chown -R + chmod -R u+rwX，checkout 时还要 git clean 爬行——为一棵已不再服务任何东西的树。
建议修复：重构使 npm ci 成功后的每条路径都到达两次清理 find … rm -rf——把快照/密封的提前退出改为 ok=false 的落穿，在 exit 0 前执行清理，或在这些出口复制清理块。修复见证：一个 workflow 套件场景，在 stub 的成功 npm ci 之后强制快照失败，并断言 node_modules/dist 已从 checkout 移除；如今没有任何东西钉住失败后的 workspace 状态。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
Deferred to the next round (batch cap). Verified real against the current script: the snapshot-failure, seal-failure, and manifest-mv-failure paths all exit 0 before the workspace cleanup at the end of the script, so a successful npm ci followed by one of those faults leaves the built tree in the checkout. The restructure (convert the early exits into fall-through cleanup) touches the same script this round already restructured for the root-ownership publish; landing both in one careful pass next round avoids re-touching the failure paths twice.
中文说明
顺延至下一轮（本轮批次上限）。已对当前脚本核实属实：快照失败、密封失败、清单 mv 失败三条路径都在脚本末尾的 workspace 清理之前 exit 0，因此 npm ci 成功后遇到其中任一故障会把已构建的树留在 checkout 中。重构（把提前退出改为落穿清理）要动的正是本轮已为 root 所有权发布重构过的同一脚本；下一轮把两处合并为一次谨慎的修改，避免对失败路径二次改动。
| expect(got.reason).toContain('.package-lock.json'); | ||
| }); | ||
|
|
||
| it('refuses a worktree that already carries a node_modules (PR content)', () => { |
There was a problem hiding this comment.
[Suggestion] R3-8: The committed-node_modules guard deliberately uses lstatSync instead of existsSync to catch a committed DANGLING symlink at node_modules (per its own comment: existsSync reads it as absent and the farm's mkdir then dies EEXIST), but the only test of this guard uses a real directory — no test places a symlink (dangling or otherwise) at the worktree's node_modules, so the very distinction the guard was written for ships unwitnessed.
Mutate the guard from lstatSync to existsSync (the more common API, already used on adjacent checks) and the whole suite stays green: a committed dangling symlink then slips past the guard and dies later in farmFromEntry's mkdir with the fallback reason 'could not create the worktree node_modules farm' instead of 'already carries a node_modules; refusing to farm over PR content' — sending the next reader of the plan report debugging an internal farm fault whose real cause is PR content.
Suggested fix:
it('refuses a committed DANGLING symlink at node_modules', () => {
const cache = tmp('depprov-cache-');
makeEntry(cache);
const wt = makeWorktree();
symlinkSync(join(wt, 'no-such-target'), join(wt, 'node_modules'));
const got = provisionWorktreeDependencies(wt, cache);
expect(got.provisioned).toBe(false);
expect(got.reason).toContain('already carries a node_modules');
});Fix witness: that test — with the guard mutated to existsSync it goes red (the dangling link reads as absent, the farm proceeds, mkdir hits EEXIST, and the reason assertion fails).
中文说明
[Suggestion] R3-8：committed-node_modules 守卫特意使用 lstatSync 而非 existsSync，以捕获提交在 node_modules 上的悬空符号链接（按其自身注释：existsSync 会把它读作不存在，农场的 mkdir 随后死于 EEXIST），但该守卫唯一的测试使用真实目录——没有任何测试在 worktree 的 node_modules 处放置符号链接（无论悬空与否），守卫为之而写的那个区分因此没有见证。
把守卫从 lstatSync 变异为 existsSync（更常见的 API，相邻检查已在用），整个套件保持绿：提交的悬空符号链接会溜过守卫，之后在 farmFromEntry 的 mkdir 处死去，回退原因变成 'could not create the worktree node_modules farm' 而不是 'already carries a node_modules; refusing to farm over PR content'——让下一个读 plan report 的人去调试一个真实原因是 PR 内容的内部农场故障。
修复见证：上述测试——守卫变异为 existsSync 后变红（悬空链接被读作不存在，农场继续，mkdir 撞上 EEXIST，原因断言失败）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // root and nested node_modules, a member dist, and the root bundle | ||
| // output the snapshot must NOT take (regenerated per release, useless to | ||
| // a test run). | ||
| mkdirSync(join(work, '.git'), { recursive: true }); |
There was a problem hiding this comment.
[Suggestion] R3-9: The fixture plants a .git directory but no assertion pins the -name .git -prune exclusions that keep it OUT of the published entry and the workspace strip; the cold-cache test asserts every other property of the entry — including the absence of the root dist — except this one, so a regression of either prune ships green.
Why it matters: the review job's 'Checkout base branch' uses actions/checkout without persist-credentials: false, so the job token sits in the checkout's .git/config. If a future edit drops \( -name .git -prune \) from the snapshot find, the next cold population hardlinks the entire .git into $HOME/.qwen-review-deps/<hash>/, where it outlives the job and is reachable by the unsandboxed PR code a later review executes on the same pool (the entry path is writable per the R1-1 class finding and computable from the PR's own lockfile) — cross-review exposure of the review job's token. The mirrored prune in the workspace-strip find regressing deletes the workspace .git mid-job just as silently.
Suggested fix: add entryGit: existsSync(join(entry, '.git')) (and workspaceGit) to the harness return and assert expect(r.entryGit).toBe(false) in the cold-cache test; the fixture already plants the directory, so the assertion is live. Fix witness: that assertion — remove \( -name .git -prune \) from either find in provision-review-deps.sh and it goes red while today's suite stays green.
中文说明
[Suggestion] R3-9：fixture 放置了 .git 目录，但没有任何断言钉住把 .git 排除在已发布条目与 workspace 清理之外的 -name .git -prune；冷缓存测试断言了条目的其他所有属性——包括根 dist 的缺席——唯独没有这一项，两处 prune 中任何一处回归都以绿通过。
为何重要：评审 job 的 'Checkout base branch' 使用 actions/checkout 且未设 persist-credentials: false，job token 就在 checkout 的 .git/config 里。若未来某次编辑从快照 find 中丢掉 \( -name .git -prune \)，下一次冷填充会把整个 .git 硬链接进 $HOME/.qwen-review-deps/<hash>/，它比 job 活得更久，且同一池上后续评审所执行的未加沙箱 PR 代码可以够到它（按 R1-1 类发现，条目路径可写且可由 PR 自己的锁文件算出）——评审 job token 的跨评审暴露。workspace 清理 find 中镜像的 prune 若回归，则会悄无声息地在 job 中途删除 workspace 的 .git。
建议修复：在 harness 返回中增加 entryGit: existsSync(join(entry, '.git'))（及 workspaceGit），在冷缓存测试中断言 expect(r.entryGit).toBe(false)；fixture 已放置该目录，断言是活的。修复见证：该断言——从 provision-review-deps.sh 任一 find 中移除 \( -name .git -prune \) 后变红，而当前套件保持绿。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
Deferred to the next round (batch cap). Verified real: the fixture plants .git, but no assertion pins either -name .git -prune — the cold-cache test asserts every other entry property except this one. Note the exposure leg of the finding (a regressed snapshot prune hardlinking .git with the job token into a writable entry) is largely closed by this round's root-ownership publish, where the entry is no longer writable on sudo members; the prune-regression witness itself (entryGit/workspaceGit assertions) still lands next round.
中文说明
顺延至下一轮（本轮批次上限）。已核实属实：fixture 放置了 .git，但没有任何断言钉住两处 -name .git -prune——冷缓存测试断言了条目的其他所有属性，唯独缺这一项。需要说明：该发现的暴露路径（快照 prune 回归、把携带 job token 的 .git 硬链接进可写条目）在本轮 root 所有权发布后已大幅收窄——在具备 sudo 的成员上条目不再可写；但 prune 回归的见证断言本身（entryGit/workspaceGit）仍将在下一轮补上。
| expect(r.stdout).toContain('free under'); | ||
| expect(r.calls).not.toContain('npm'); |
There was a problem hiding this comment.
[Suggestion] R3-10: The disk gate skips the install and exits 0 BEFORE the script's only reclamation legs — the entry prune and the torn-stage sweep both run after the successful publish the gate blocks — and no test exercises reclamation under disk pressure. The test title 'the disk gate skips the install, never the job' pins exactly this ordering as intended, but the ordering lets the condition that trips the gate persist forever.
Concrete latch: timeout-minutes: 30 kills a population run mid-npm ci against a flaky registry, leaving a ~2G torn .stage.* dir; a few repeats push free space under $HOME below the 10G threshold. Every later run hits the disk gate and exits before the prune (find … tail -n +4) and the sweep (find … -mmin +240 -exec rm -rf) — the only lines that could reclaim the very garbage that trips the gate — so the runner can never populate again and never cleans up, permanently falling back to the uncached behaviour this step exists to eliminate.
Suggested fix: run the prune + stale-stage sweep before the disk gate in provision-review-deps.sh (reclaim first, then decide), and add a scenario: seed four stale entries plus an old torn .stage.x (backdated with touch -d @old), run with the low-disk df stub, and assert the oldest entry and torn stage were removed even though the install was skipped. Fix witness: that scenario — red under today's ordering (the gate exits before reclaim) and green once reclaim precedes the gate.
中文说明
[Suggestion] R3-10：磁盘闸门跳过安装并在脚本唯一的回收腿之前 exit 0——条目剪枝与撕裂阶段清扫都在闸门所阻止的成功发布之后运行——且没有任何测试在磁盘压力下执行回收。测试标题 'the disk gate skips the install, never the job' 把这一顺序钉为有意设计，但该顺序让触发闸门的条件永远持续。
具体闩死：timeout-minutes: 30 在一次对不稳注册表的 npm ci 中途杀掉填充运行，留下约 2G 的撕裂 .stage.* 目录；几次重复后 $HOME 空闲空间跌破 10G 阈值。之后每次运行都命中磁盘闸门，并在剪枝（find … tail -n +4）与清扫（find … -mmin +240 -exec rm -rf）——唯一能回收触发闸门的那些垃圾的行——之前退出，于是该 runner 永远无法再填充、也永远不再清理，永久回退到本步骤正要消除的无缓存行为。
建议修复：在 provision-review-deps.sh 中把剪枝 + 陈旧阶段清扫放到磁盘闸门之前（先回收再决定），并新增场景：种入四个陈旧条目加一个旧的撕裂 .stage.x（用 touch -d @old 回拨时间），用低磁盘 df stub 运行，断言即便跳过安装，最旧条目与撕裂阶段也被移除。修复见证：该场景——在当前顺序下变红（闸门先于回收退出），回收前置后变绿。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
Deferred to the next round (batch cap). Verified real against the current script: the disk gate exits before the entry prune and the torn-stage sweep, so the garbage that trips the gate persists and the only reclamation legs never run under disk pressure. The reorder (reclaim first, then decide) plus the low-disk reclamation scenario is a self-contained change to the same script this round restructured for the root-ownership publish; landing it next round keeps each failure-path edit to a single careful pass.
中文说明
顺延至下一轮（本轮批次上限）。已对当前脚本核实属实：磁盘闸门在条目剪枝与撕裂阶段清扫之前退出，触发闸门的垃圾因此持续存在，唯一的回收腿在磁盘压力下永远不会运行。顺序调整（先回收再决定）加低磁盘回收场景，是对本轮已为 root 所有权发布重构过的同一脚本的一个独立改动；下一轮落地，使每条失败路径的修改都只经过一次谨慎的改动。
…ticals (#10129) Trust separation replaces content attestation as the cache's primary seal: on pool members with passwordless sudo the publish elevates the cache root and every entry to root ownership (warn-and-degrade where sudo is absent), closing the cross-review poisoning class R1-1 proved reachable. The farm re-checks the resolved entry stays under the cache root, contains the rebuilt .bin on both link text and member bin name/target, checks demand range satisfaction against the lockfile the entry carries plus the root manifest, and restores owner-write on the dist copies a scoped build overwrites. Each guard ships with its own mutation-proven witness.
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #10129 (address-review)Growth audit (required this window — over test-line budget)Recorded in Findings addressed
R1-1 — cross-review cache poisoning (Critical, fix-induced)The primary fix is trust separation, per the finding's own suggested shape:
R1-16 — demand check was name-presence only (Critical, fix-induced)
R1-25 —
|
| ID | 级别 | 处理 |
|---|---|---|
| R1-1 (rc:3875891064) | Critical | 已修复 |
| R1-16 (rc:3875891068) | Critical | 已修复 |
| R1-25 (rc:3875891074) | Critical | 已修复 |
| R3-1 (rc:3875891082) | Critical | 已修复 |
| R3-3 (rc:3875891099) | Suggestion | 已修复 |
| R3-5 (rc:3875891111) | Suggestion | 已修复 |
| R3-6 (rc:3875891116) | Suggestion | 已修复 |
| R3-8 (rc:3875891129) | Suggestion | 已修复 |
| R3-2、R3-4、R3-7、R3-9、R3-10 | Suggestion | 顺延至下一轮（批次上限约 8 条；已在各自线程回复） |
R1-1 — 跨评审缓存投毒（Critical，修复引入）
主修复即该发现自身建议形态的信任分离：
.github/scripts/provision-review-deps.sh：在具备密码less sudo 的池成员上（用工作流既有的sudo -n true模式探测一次），发布流程现在先把暂存文件密封为只读，再sudo -n chown -R root:root暂存目录、sudo -n chown root:root+chmod go-w缓存根、sudo -n mv -T落位。本 job 接下来执行的未加沙箱 PR 代码保留农场所需的读权限，失去 R1-1 已证明的全部写通道（原位重写、解除链接再重建、整体替换、在条目名上植入符号链接——全部需要对缓存根的写）。剪枝/清理删除与热路径重建经由带 sudo 腿的drop_path辅助函数执行；提权失败时丢弃暂存并告警，绝不让 job 失败。没有 sudo 的成员保持与之前完全一致的用户所有发布，清单重校验作为已披露的兜底保留。dep-provision.ts：provisionWorktreeDependencies现在在解析条目名后复查insideDir(realpathSync(cacheRoot), entry)——植入在条目名上的符号链接会被拒绝（R1-1 CHANNEL2）。根.bin重建现在对每个镜像链接应用同一条容纳判定：解析到条目之外的链接文本计入失败，绝不逐字复制（R1-1 通道 a）。- 见证：一个 shell 场景用记录式 sudo 垫片驱动真实脚本，断言每条提权腿都执行且条目仍然完整密封落位；变异 M9（禁用提权分支）使其变红。TS 测试拒绝植入符号链接的条目与外逃的
.bin文本；变异 M1/M4 使其变红。 - 有证据地拒绝的子项：附加的「把符号链接集合纳入封印」内容证明。在有 sudo 的成员上，root 所有权已经移除了该封印要防的写；在无 sudo 的成员上，条目整体仍可被攻击者写，任何符号链接封印本身也是攻击者可写的，落入该发现对清单所证的同一引理（写的一方可以重新撰写被校验的状态）。加上它只会让 diff 增长换来无法成立的防御。
R1-16 — 需求检查只查名称存在性（Critical，修复引入）
- 需求检查现在对照农场已持有的锁文件校验范围满足：对每个拥有 semver 有效 spec 且锁文件在
packages['node_modules/<dep>'].version记录了版本的被需求依赖，semver.satisfies必须接受，否则该成员计入失败。根清单以相同的可选依赖排除处理加入检查，因此没有workspaces字段的仓库不再无人检查。 - 相对建议中「无法校验即失败回退」的记录在案偏差：非 semver spec（
file:、git+、链接）与未记录版本的锁文件条目保持存在性检查——这正是 npm 对这些 spec 的退化检查。对它们失败回退会使任何使用 git/file 依赖的仓库永久无法供给，为修复一个 npm 自身都不拒绝的类别而回归本特性的目的（review: provision worktree dependencies from a host cache on the persistent runners #10108）。 - 见证：锁文件记录
plain-pkg@1.0.0而成员要求^2.0.0时返回provisioned: false, failed > 0且无 npm 标记；^1.0.0的孪生用例通过；根清单要求条目缺失依赖时农场失败，含无workspaces字段的仓库形态。变异 M2/M3 使其变红，而当前套件保持绿。
R1-25 — .bin 重推导使用未受约束的 bin 名/目标（Critical，修复引入）
- 每个成员 bin 现在在任何文件系统操作之前双向受约束：链接名必须解析在
.bin之内，bin 的解析目标必须留在其自身成员目录之内；否则该成员计入失败，不 rm/创建任何东西。 - 见证：
"bin": { "../evil": "./cli.js" }与"bin": { "corebin": "../../escaped-cli.js" }都计入失败且农场之外无任何创建。变异 M5 使其变红。
R3-1 — 只读条目文件破坏定向重建（Critical）
cpSync复制预构建dist之后，农场遍历副本并按文件恢复所有者写权限（mode | 0o200）。共享条目保持不可变；worktree 副本可构建。- 见证：fixture 现在把条目密封为只读（与填充步骤一致），'farms a matching entry' 额外成功写入一个复制出的 dist 文件，同时条目自身的副本保持原样。变异 M6 使其变红——此前整个交互以绿通过。
R3-3、R3-5、R3-6、R3-8（Suggestion，已修复）
- R3-3：'populates a cold cache' 现在断言已发布载荷文件的模式不含任何写位（在 fixture 拆除前于 harness 内捕获），且无 sudo 时未发生任何提权。变异 M8（删除 chmod 行）使其变红。
- R3-5：三个新需求测试——未满足的
devDependencies条目使农场失败且无标记；满足的devDependencies条目通过；peerDependenciesMeta标记为可选的 peer 缺失时仍成功。变异 M7（从需求集合移除devDependencies）使未满足用例变红。 - R3-6：顺序测试现在先断言
'Resolve PR context'锚存在再比较索引。变异 M10（YAML 中重命名锚）使其变红；探针后已恢复。 - R3-8：新测试以确切的 reason 拒绝提交在
node_modules上的悬空符号链接；把守卫从lstatSync变异为existsSync使其变红。
顺延至下一轮（批次上限；已发布 comment-replies.json）
R3-2（provisionSourceOf 测试不可达）、R3-4（文件计数分支无见证）、R3-7（npm ci 成功后的失败路径跳过 workspace 清理）、R3-9（.git prune 无见证）、R3-10（磁盘闸门先于回收退出）。五条均已对当前代码核实属实；每条回复说明了核实内容与顺延理由。关于 R3-9：其暴露路径已被本轮的 root 所有权发布大幅收窄；见证本身仍在下一轮补上。
评审体备注（rv:5045694218）
其再次确认的四条早前 Suggestion（R1-5 LRU mtime、R1-7 KEEP-IN-SYNC 字面钉、R1-13 OCCUPIED 分支覆盖、R2-2 哈希断言）仍是早前轮次的开放线程，不在本轮可执行集合内。收敛观察（发现反复回到同一批文件）正是 drift 结论所处理的：信任分离移除了逐通道守卫一直在处理的共同根因。
失败 CI 检查 — 诊断
Test (ubuntu-latest, Node 22.x) 与 Post Coverage Comment（Test 的下游）在上次推送时失败。本环境拿不到 CI 日志，因此诊断为本地复现：没有任何 PR 相关失败可以复现。本地全量套件的每一类失败都追溯到本沙箱的具体环境因素，且各自通过只翻转一个变量得到证明：
- 本 autofix 会话自身的
QWEN_*/SANDBOX环境变量泄漏进 shell：config/settings 套件在变量存在时失败，unset 后 529/529 通过。 $HOME（/home/github-runner）为 root 所有、测试用户不可写：workflow-snapshot/gemini 套件以~/.qwen上的 EACCES 失败，HOME=/tmp/fakehome后通过。- 早前会话遗留的 root 所有
/tmp/qwen-code-test-root使 write-file 测试以 EPERM 失败；无 root 无法移除。 - 并发运行两个全量套件会使覆盖率序列化崩溃——失败集合在各次运行间迁移，符合资源争用而非代码缺陷。
- scripts 套件的 5 个闪失（triage 代理看门狗、verify-capture 颜色）位于本 PR 从未触碰的文件中，单独运行均通过——并发负载闪失。
结论：红色检查最可能是干净 CI 上同一争用/闪失类别；PR 自身的套件在所有可运行之处均为绿。工作流的独立 CI 仍是最终闸门。
冲突
--conflict false：未执行任何合并。
验证
本轮实际执行的命令（结果）：
npm run build— 通过（退出码 0）npm run typecheck— 通过（退出码 0）npm run lint（ESLint）— 通过（退出码 0）npx prettier --check（dep-provision.ts / dep-provision.test.ts / qwen-pr-review-workflow.test.js）— 通过bash -n .github/scripts/provision-review-deps.sh— 通过；可执行位由 'ships the script executable' 钉住vitest run dep-provision.test.ts（触碰文件）— 38/38 通过vitest run dep-provision.test.ts worktree.test.ts（触碰文件）— 107/107 通过vitest run teamHelpers.test.ts（触碰文件）— 通过npm run test:scripts— qwen-pr-review-workflow.test.js 依赖缓存套件 14/14 绿（定向），全文件 1820 通过 / 16 跳过 / 5 个位于本 PR 未触碰的 triage+verify-capture 文件的闪失（单独运行通过——见诊断）- 变异探针 M1–M10（本轮新增的每个守卫都有自己的见证）：M1 channel2 容纳、M2 根需求、M3 范围满足、M4 条目
.bin容纳、M5 成员 bin 容纳、M6 chmod 恢复、M7 devDependencies 需求、M8 chmod a-w 密封、M9 提权分支、M10 YAML 锚——每条：移除/取反守卫 → 定向测试失败 → 恢复 → 绿 - 本环境不可运行（披露，非失败）：shellcheck/actionlint/yamllint 二进制无法在此安装（无可写的 linter 缓存目录、缺少
xz）；YAML 改动仅在断言侧，脚本已通过 bash -n。全 workspace 套件因上述已证明的环境原因在本沙箱不可靠；未运行集成测试——本轮行为完全位于单测覆盖的供给器与真实 bash 套件中，且 fetch-pr 接线未变。
Deferred non-Critical feedback
Critical-only mode is active: the PR's diff grew src 379 / test 498 net lines beyond this counting window's baseline (budgets: 400/400). The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback continues to flow unaffected during a growth-only engagement (the per-author batch budget applies only after 5 change-producing rounds). (@qwen-code /retry starts a fresh counting window.)
中文说明
已进入仅处理 Critical 的模式：本计数窗口内 diff 净增长已达 源码 379 / 测试 498 行（预算 400/400）。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。纯增长触发期间维护者反馈照常流动（按作者的批次预算仅在完成 5 个产生改动的轮次后生效）。（评论 @qwen-code /retry 可开启新的计数窗口。）
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。
Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。
🧠 Handled by Qwen Code · model/模型 qwen3.8-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-4 — skipped workspace members get no self-link or count — already reported (round 1, open thread on dep-provision.ts:325)
- R1-6 — fetch-pr step 4b wiring (sole production caller and dependencies-field writer) untested — already reported (round 1, open thread on fetch-pr.ts:1020)
- R1-7 — KEEP-IN-SYNC contract literals pinned on one side only, including the unpinned manifest/source-rev filenames — already reported (round 1, open thread on dep-provision.ts:109; listed as already reported in the round-3 summary)
- R1-13 — self-link loop's legitimate OCCUPIED branch has no coverage — already reported (round 1, open thread on dep-provision.test.ts)
- R1-21 — harness feeds the script's bytes to bash -c; the exec bit and shebang the production invocation depends on are unpinned — already reported (round 1, open thread on qwen-pr-review-workflow.test.js)
- R1-24 — final guard's tally.linked === 0 arm untested — already reported (round 1, open thread on dep-provision.ts)
- R2-8 — step.if pinned but the runs-on pool selector it mirrors is asserted nowhere — already reported (round 2, open thread on qwen-pr-review-workflow.test.js:4337, deferred by the author to this round)
Not reviewed: reverse audit — stopped at the 5-round cap without converging; rounds 1–5 each reported findings.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/dep-provision.ts:254 — [review] behind-main PRs always fall back; the class is absent from the PR's fallback enumeration.github/scripts/provision-review-deps.sh:104 — [probe] torn-stage sweeps unreachable from the warm path (and every early exit)scripts/tests/qwen-pr-review-workflow.test.js:4205 — [review] the suite never runs the script's own publish into the script's own warm checkpackages/cli/src/commands/review/lib/dep-provision.test.ts:605 — [probe] entryManifestValid's listed-but-missing arm (listed.size === 0) has no testpackages/cli/src/commands/review/lib/dep-provision.ts:413 — [probe] demand check never reads optionalDependencies, but npm ci's sync check validates themscripts/tests/qwen-pr-review-workflow.test.js:4030 — [review] fixture node_modules has zero symlinks while real entries always carry workspace linksscripts/tests/qwen-pr-review-workflow.test.js:4134 — [probe] the dist-snapshot find's -name node_modules prune clause is unpinnedpackages/cli/src/commands/review/lib/dep-provision.ts:424 — [probe] demand check false-negatives for a depended-on member the snapshot never materialisedpackages/cli/src/commands/review/lib/dep-provision.ts:323 — [probe] graph.skipped members with unparseable manifests escape the demand passpackages/cli/src/commands/review/lib/worktree.ts:1613 — [review] provisionRoot predicate's nested-node_modules arm has zero test coverage
Convergence: round 4 posted 13 inline comment(s), 8 of them reported for the first time; the previous round posted 13 (13 new). Findings keep coming back to the same files: .github/scripts/provision-review-deps.sh (findings in rounds 1, 3; 4 more now); packages/cli/src/commands/review/lib/dep-provision.ts (findings in rounds 1, 3; 4 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 7 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without converging; rounds 1–5 each reported findings。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 10 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 13 条行内评论，其中 8 条是首次提出；上一轮发布了 13 条（其中 13 条首次提出）。发现反复回到同一批文件：.github/scripts/provision-review-deps.sh（第 1、3 轮已出过发现，本轮又有 4 条）；packages/cli/src/commands/review/lib/dep-provision.ts（第 1、3 轮已出过发现，本轮又有 4 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| # Immutable files BEFORE the elevation (the user still owns the stage), | ||
| # so a degraded re-publish never serves writable payloads either. | ||
| find "$STAGE" -type f -exec chmod a-w {} + 2>/dev/null | ||
| if sudo -n chown -R root:root "$STAGE" 2>/dev/null && |
There was a problem hiding this comment.
[Critical] R1-1: (fix-induced) The round-4 root-own fix closed the entrance this thread reported — unprivileged in-place tamper of the sealed entry — but the elevated publish it added leaves the published entry unreadable by the runner user, so the mechanism never works on exactly the pool members it exists for. The mktemp -d stage top directory is chowned to root without ever opening its mode (drwx------ root:root — find -type f -exec chmod a-w touches only files), and the manifest — mktemp'd under $HOME at 0600 and only chmod a-w before elevation — publishes as 0400 root:root.
Executed end-to-end with real sudo on this host against the unmodified script: the runner user gets EACCES on cat/ls of the entry, [ -f marker ] is false; the real provisionWorktreeDependencies returns provisioned: false "carries no completeness marker" permanently; the producer's second run re-runs a full npm ci and then fails re-publish (mv -T cannot rename over the occupied unreadable entry). The elevation test stubs sudo (no real elevation) and asserts only file writability, so it cannot catch this.
witness (unmodified PR, real sudo): entry dir 700 root:root; runner-user cat/ls DENIED;
consumer: provisioned:false "carries no completeness marker"; run2: "elevation failed; the entry was not published"
with the fix (stage chmod 755 + manifest opened): entry dir 755, manifest 444, consumer provisioned:true, warm hit, one install
Open the stage's top directory and the manifest's read bits before elevation: chmod 755 "$STAGE" right after each mktemp -d, and chmod a+r the manifest right after the mv into the stage — the seal's integrity comes from root ownership, not from the entry being unreadable. The elevation scenario must additionally assert the published entry's top dir grants non-owner read+execute and the manifest is group/other-readable; removing either chmod must turn it red.
中文说明
[Critical] R1-1：（修复引入）第 4 轮的 root 所有权修复关闭了本线程报告的入口——对已密封条目的无特权就地篡改——但它新增的提权发布把发布出的条目变得对 runner 用户不可读,于是该机制恰恰在它为之存在的池成员上从不工作。mktemp -d 的 stage 顶层目录被 chown 给 root 却从未放开权限位（drwx------ root:root——find -type f -exec chmod a-w 只处理文件）;而 manifest——在 $HOME 下由 mktemp 以 0600 创建、提权前仅 chmod a-w——发布后是 0400 root:root。
已在本机用真实 sudo 对未修改的脚本端到端执行:runner 用户对条目 cat/ls 得到 EACCES,[ -f marker ] 为 false;真实的 provisionWorktreeDependencies 永久返回 provisioned: false "carries no completeness marker";生产端第二次运行重新执行完整 npm ci,随后重新发布失败（mv -T 无法重命名覆盖被占用且不可读的条目）。elevation 测试把 sudo 打桩（无真实提权）且只断言文件可写,因此抓不到这个问题。
修复:提权前放开 stage 顶层目录与 manifest 的读权限——每个 mktemp -d 之后加 chmod 755 "$STAGE",manifest mv 进 stage 之后加 chmod a+r——密封的完整性来自 root 所有权,而不是条目不可读。elevation 场景还需断言发布条目的顶层目录对非属主可读可执行、manifest 对组/其他可读;删掉任一 chmod,测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| set -u | ||
| set +e | ||
| CACHE_ROOT="$HOME/.qwen-review-deps" | ||
| mkdir -p "$CACHE_ROOT" || exit 0 |
There was a problem hiding this comment.
[Critical] R4-8: Every privileged operation in this script escalates THROUGH $CACHE_ROOT without ever re-verifying the root's own identity, and the root sits in a user-writable, non-sticky parent ($HOME). Prior unsandboxed PR code on the same persistent host can rename the root-owned cache root away and replace it, and the script's own sudo then serves or escalates through the swap — the root-ownership seal this round added never checks that it is still sealing the same directory.
Exploit chain executed on this host: mv ~/.qwen-review-deps ~/.q && mkdir ~/.qwen-review-deps succeeds as the unprivileged user — rename(2) checks write permission on the PARENT ($HOME, user-owned), not ownership of the directory, so chmod go-w on the root never blocks it. The attacker then plants an entry for the sha256 of the public base lockfile with a self-consistent manifest, .qwen-review-source-rev at the current public main tip, the completeness marker, and a poisoned node_modules. The next review: mkdir -p is a no-op, entry_verifies passes attacker-authored state, the warm path serves it, and every farm-side check (lockfile byte-compare, manifest re-hash, source-rev) passes self-authored state — probes execute attacker code in a job holding pull-requests: write / issues: write, persistently. Symlink variant: ln -s <victim> ~/.qwen-review-deps makes the script's own sudo chown root:root / chmod go-w land on an arbitrary directory.
witness: unprivileged mv renamed the 755 root:root cache root and replaced it;
sudo chown/chmod via planted symlink → "755 root:root victim-dir";
real script vs planted entry → "dependency cache warm: …", payload console.log("attacker code"); intact
Re-establish the root's identity at the top of every run, before any trust or escalation: require [ -d "$CACHE_ROOT" ] && [ ! -L "$CACHE_ROOT" ], and on sudo members additionally require root ownership (stat -c %u = 0); on any failure move the occupant aside (rename works without sudo via the parent) and mkdir -p a fresh root. Never run the elevation chown/chmod on a path that has not passed this check. A test scenario whose prepare replaces the fixture home's cache root with a symlink (and with a planted user-owned entry) should assert the step does not serve or publish through it.
中文说明
[Critical] R4-8: 脚本中所有提权操作都"穿过" $CACHE_ROOT 执行,却从不重新验证根目录自身的身份,而根目录位于用户可写、非 sticky 的父目录（$HOME）中。同一持久主机上先前的未沙箱 PR 代码可以把 root 所有的缓存根目录重命名挪走并替换,脚本自己的 sudo 随后就会为这个替换物提供服务或对其提权——本轮新增的 root 所有权密封从不检查它密封的还是不是同一个目录。
已在本机执行完整利用链:非特权用户执行 mv ~/.qwen-review-deps ~/.q && mkdir ~/.qwen-review-deps 成功——rename(2) 检查的是父目录（$HOME,用户所有）的写权限,而不是被重命名目录的属主,所以对根目录的 chmod go-w 拦不住它。攻击者随后为公开 base 锁文件的 sha256 植入一个条目:自洽的 manifest、指向当前公开 main tip 的 .qwen-review-source-rev、完整性标记,以及投毒的 node_modules。下一次评审:mkdir -p 无效操作,entry_verifies 通过攻击者自造的状态,热路径直接服务该条目,农场端所有检查（锁文件字节比对、manifest 重哈希、source-rev）都通过自造状态——探针在持有 pull-requests: write / issues: write 的 job 里执行攻击者代码,且可持久复现。符号链接变体:ln -s <victim> ~/.qwen-review-deps 会让脚本自己的 sudo chown root:root / chmod go-w 落在任意目录上。
修复:每次运行开始、任何信任或提权之前重建根目录身份:要求 [ -d "$CACHE_ROOT" ] && [ ! -L "$CACHE_ROOT" ],sudo 成员上额外要求 root 所有（stat -c %u = 0）;任一失败则把占用者挪走（经父目录重命名无需 sudo）并 mkdir -p 新根。未通过该检查的路径绝不执行提权 chown/chmod。测试场景可在 prepare 中把 fixture home 的缓存根替换为符号链接（及植入的用户所有条目）,断言步骤不为其服务、不向其发布。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| manifest.dependencies?.[dep] ?? | ||
| manifest.devDependencies?.[dep] ?? | ||
| manifest.peerDependencies?.[dep]; | ||
| const locked = lockPackages?.[`node_modules/${dep}`]; |
There was a problem hiding this comment.
[Critical] R4-9: The member demand check compares a manifest range only against the TOP-LEVEL lock entry (node_modules/<dep>), never against the member-nested entry where npm actually resolves the edge — and on this repo's own lockfile that fails the demand pass for workspace members, so the farm falls back for EVERY PR and the cache mechanism is a permanent no-op here. The new RANGED_LOCK tests model only a single top-level version, so the suite is green while the feature is dead.
Measured against this repo at HEAD with the repo's own semver: packages/core demands ajv@^8.17.1; npm resolves that edge to packages/core/node_modules/ajv@8.20.0 while the top level holds node_modules/ajv@6.15.0; semver.satisfies('6.15.0', '^8.17.1') is false → demandUnmet → provisioned: false → every review falls back to exactly the npm ci this PR exists to remove. Full sweep: 38 (member, dep) pairs across 12 workspace members fail this check while npm's actual resolution satisfies the range in all 38 (same shape: packages/cli markdown-it ^15.0.0 vs top-level 14.2.0).
witness: sweep — "TOTAL false-fail (member,dep) pairs: 38 | of which npm ci would SUCCEED: 38 | members affected: 12"
semver.satisfies(top 6.15.0, ^8.17.1) = false; semver.satisfies(nested 8.20.0, ^8.17.1) = true
Consult the lock entry where npm resolves the edge: in the member pass, prefer the member-nested key (<memberRelDir>/node_modules/<dep> — the demand loop already has the member dir) and fall back to node_modules/<dep>; keep the root pass top-level-only. Add a test with a lockfile recording node_modules/plain-pkg@1.0.0 AND packages/core/node_modules/plain-pkg@2.0.0, a member manifest demanding plain-pkg@^2.0.0 → expect provisioned: true; with the top-level-only lookup it returns false, and removing the nested-preference lookup must turn the test red.
中文说明
[Critical] R4-9: 成员需求检查只把 manifest 版本范围与顶层锁条目（node_modules/<dep>）比较,从不与 npm 实际解析该边所用的成员嵌套条目比较——而按本仓库自己的锁文件,这会让工作区成员的需求检查失败,于是农场对每个 PR 都回退,缓存机制在本仓库永远是空转。新增的 RANGED_LOCK 测试只建模单一顶层版本,所以套件全绿而功能已死。
在 HEAD 上用仓库自己的 semver 实测:packages/core 需要 ajv@^8.17.1;npm 将该边解析到 packages/core/node_modules/ajv@8.20.0,而顶层是 node_modules/ajv@6.15.0;semver.satisfies('6.15.0', '^8.17.1') 为 false → demandUnmet → provisioned: false → 每次评审都回退到本 PR 要消灭的那个 npm ci。全量扫描:12 个工作区成员共 38 个（成员,依赖）对未通过该检查,而 npm 的真实解析在全部 38 对中都满足范围（同形:packages/cli 的 markdown-it ^15.0.0 对顶层 14.2.0）。
修复:按 npm 解析该边的位置查锁条目——成员路径优先成员嵌套键（<成员相对目录>/node_modules/<dep>,需求循环手里已有成员目录）,回退到 node_modules/<dep>;root 路径保持只查顶层。补测试:锁文件同时记录 node_modules/plain-pkg@1.0.0 与 packages/core/node_modules/plain-pkg@2.0.0,成员 manifest 需要 plain-pkg@^2.0.0 → 期望 provisioned: true;只查顶层时返回 false,删掉嵌套优先逻辑测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| tally.failed++; | ||
| } | ||
|
|
||
| if (tally.failed > 0 || tally.linked === 0) { |
There was a problem hiding this comment.
[Critical] R4-12: The final completeness gate only checks failed > 0 || linked === 0; nothing reconciles the derived workspace graph against the members the lockfile records, so a silently-empty graph yields provisioned: true plus the npm completeness marker for a tree missing every self-link and every member dist. The comment near memberDirs asserts a "linked-vs-expected check" that does not exist anywhere in the function, and the lockfile already in the function's hand (parseLockfilePackages lists every member as link: true) is never consulted.
Trigger reachable today: a PR corrupts or deletes root package.json (bad conflict resolution) without touching package-lock.json — the lockfile hash still matches the warm entry and the source-rev gate passes. readWorkspacePackages catches parse failures internally (so the tally.failed++ catch is dead for these shapes), the root demand check's catch records nothing, the root farm still links every third-party package so linked > 0, and the gate stamps node_modules/.package-lock.json. build-test then skips npm ci (its gate is exactly that marker) and probes run against a tree with zero @qwen-code/* self-links and zero prebuilt member dist — module errors attributed to the PR with the plan report recording provisioned: true, where the cold path would have failed loudly with EJSONPARSE. Latent variant: lockfile workspaces globs workspaceDirCandidates cannot expand (packages/**, packages/foo-*) farm zero members while claiming completeness.
witness (probe, real provisioner): intact graph → provisioned:true linked:2 selfLinked:1;
corrupt-root / deleted-root → provisioned:true linked:1 selfLinked:0 distCopied:0, npm marker PRESENT;
minimal fix (tally.failed++ in the root-manifest catch) → both arms provisioned:false failed:1
Reconcile the graph against the lockfile's own member record before the gate: for every link: true entry in parseLockfilePackages(lockBytes), require a corresponding self-linked member or count the shortfall into tally.failed; minimally, tally.failed++ when the root manifest is unreadable and fall back when hasUnmodeledWorkspaceGlob(readWorkspaceGlobs(worktreeReal)) is true. Also fix or delete the comment describing the nonexistent check. Add warm-entry scenarios with an unparseable root package.json and with workspaces: ["packages/**"], asserting provisioned === false and no npm marker; removing the reconciliation must turn them red.
中文说明
[Critical] R4-12: 最终完整性闸门只检查 failed > 0 || linked === 0;没有任何环节把推导出的工作区图与锁文件记录的成员对账,于是静默为空的图会为缺少全部 self-link 和全部成员 dist 的树盖上 provisioned: true 和 npm 完整性标记。memberDirs 附近的注释声称存在一个"linked-vs-expected 检查",但函数里根本不存在;函数手里已有的锁文件（parseLockfilePackages 把每个成员列为 link: true）也从未被查询。
今天就可触发的场景:PR 损坏或删除根 package.json（糟糕的冲突解决）而不动 package-lock.json——锁文件哈希仍匹配热条目,source-rev 闸门通过。readWorkspacePackages 内部吞掉解析失败（因此 tally.failed++ 的 catch 对这些形态是死代码）,根需求检查的 catch 什么都不记录,根农场仍会链接全部第三方包使 linked > 0,闸门盖上 node_modules/.package-lock.json。build-test 随后跳过 npm ci（它的闸门正是这个标记）,探针在没有任何 @qwen-code/* self-link、没有任何预构建成员 dist 的树上运行——模块错误被记到 PR 头上,而 plan report 记录 provisioned: true;冷路径本会以 EJSONPARSE 大声失败。潜在变体:锁文件中 workspaceDirCandidates 无法展开的 workspaces 通配（packages/**、packages/foo-*）会农场零成员却声称完整。
修复:闸门前用锁文件自身的成员记录对账——对 parseLockfilePackages(lockBytes) 中每个 link: true 条目,要求存在对应 self-link 的成员,否则把缺口计入 tally.failed;最小修复为根 manifest 不可读时 tally.failed++,并在 hasUnmodeledWorkspaceGlob(readWorkspaceGlobs(worktreeReal)) 为真时回退。同时修正或删除描述不存在检查的注释。补热条目场景:不可解析的根 package.json、workspaces: ["packages/**"],断言 provisioned === false 且无 npm 标记;删掉对账逻辑测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| } | ||
| let recorded: string; | ||
| try { | ||
| recorded = readFileSync( |
There was a problem hiding this comment.
[Critical] R4-13: provisionSourceOf reads the PR-controlled farm marker (node_modules/.qwen-review-farm) with an unbounded, symlink-following readFileSync BEFORE any validation — a committed symlink marker pointing at /dev/zero (or a writerless FIFO) crashes or hangs the review CLI. The module's own discipline is lstatSync (not follow-reads) precisely because force-added node_modules content is untrusted — the committed-node_modules guards a few lines up say so; this read is the one spot that skips it.
A malicious PR force-adds two gitignored paths: node_modules/.package-lock.json (any content — it makes installComplete() true, so build-test skips the npm ci that would have wiped the planted tree) and node_modules/.qwen-review-farm → symlink → /dev/zero (a 9-byte git object). provisionWorktreeDependencies refuses to farm over the committed node_modules — no crash there — but every probe that creates a scratch tree reaches exposeDependencies, whose containment build calls provisionSourceOf; with QWEN_REVIEW_DEPS_CACHE always set in CI, the readFileSync follows the symlink. A writerless FIFO hangs in open until the job timeout; /dev/zero grows memory until a fatal uncatchable abort. Any PR, with committed content alone and nothing executed, kills its own review pipeline on the persistent runners this PR targets. test-efficacy.ts:1755 is a second consumer of the same read.
witness: FIFO arm through the real function (marker symlink → writerless FIFO): no return, timeout killed it (exit 124);
/dev/zero arm at the exact readFileSync call under a 4 GB cap: "terminate called after throwing an instance of 'std::bad_alloc'" + core dump — uncatchable by the function's try/catch;
with the fix (lstat first, isFile() && size <= 4096): both arms return null, healthy path still validates
lstatSync the marker first and admit only a regular file of bounded size before reading (inside the existing try/catch), then readFileSync. Add a provisionSourceOf test planting the marker as a symlink to a regular file containing a valid entry path and expect null; without the guard the read follows the link, so removing it turns the test red.
中文说明
[Critical] R4-13: provisionSourceOf 在任何校验之前就用无界、跟随符号链接的 readFileSync 读取 PR 可控的农场标记（node_modules/.qwen-review-farm）——committed 的符号链接标记指向 /dev/zero（或无写者的 FIFO）就能让评审 CLI 崩溃或挂起。本模块自己的纪律是 lstatSync（而不是跟随读取）,正因为强添加的 node_modules 内容不可信——上面几行的 committed-node_modules 守卫就是这么说的;这处读取是唯一跳过该纪律的地方。
恶意 PR 强添加两个被 gitignore 的路径:node_modules/.package-lock.json（任意内容——它使 installComplete() 为真,于是 build-test 跳过本可清掉植入树的 npm ci）和 node_modules/.qwen-review-farm → 符号链接 → /dev/zero（9 字节的 git 对象）。provisionWorktreeDependencies 会拒绝在 committed 的 node_modules 上农场化——那里不会崩——但每个创建 scratch 树的探针都会到达 exposeDependencies,其 containment 构建调用 provisionSourceOf;CI 中 QWEN_REVIEW_DEPS_CACHE 总是已设置,readFileSync 跟随符号链接。无写者 FIFO 会在 open 中挂到 job 超时;/dev/zero 会让内存增长直到致命的不可捕获中止。任何 PR 仅凭 committed 内容、无需执行任何东西,就能在这个 PR 所针对的持久 runner 上杀掉自己的评审流水线。test-efficacy.ts:1755 是同一读取的第二个消费者。
修复:先 lstatSync 标记,只接受有界大小的普通文件再读取（放在现有 try/catch 内）,然后 readFileSync。补一个 provisionSourceOf 测试:把标记植入为指向含合法条目路径的普通文件的符号链接,期望 null;没有守卫时读取会跟随链接,删掉守卫测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| expect(provisionSourceOf(makeWorktree())).toBe(null); | ||
| }); | ||
|
|
||
| it('rejects a marker naming a path inside the dependency root', () => { |
There was a problem hiding this comment.
[Suggestion] R3-2: still stands — the file was changed this round, but these three tests still call provisionSourceOf without setting DEPS_CACHE_ENV. 'rejects a marker naming a path inside the dependency root' (here), 'rejects a marker naming a directory without the completeness marker' and 'rejects a relative or empty recorded path' each return at the function's FIRST guard (if (!cacheRoot) return null;) and never execute the security guards their names and comments claim to pin. The sibling test immediately above ('rejects a marker naming a valid entry OUTSIDE the configured cache') wraps its call in withDepsCacheEnv, showing the pattern the other three omit.
QWEN_REVIEW_DEPS_CACHE is unset throughout every vitest run of this file (nothing in the repo sets it for tests), so deleting any of the three production guards — if (insideDir(rootReal, real)) return null;, the DEPS_COMPLETE_MARKER existsSync, or the !recorded || !isAbsolute(recorded) check — leaves the entire suite green. Those guards are what stop a force-committed .qwen-review-farm marker from widening scratch-tree containment to a PR-chosen path.
Wrap each call in withDepsCacheEnv(<an existing tmp dir>, () => …) so the intended guard is the deciding one; for the completeness-marker test, place the marker-less directory INSIDE the cache root so the flow passes cache-root containment and fails only on the missing marker. With the env wrapped, removing each guard must turn the corresponding test red — as written, none can.
中文说明
[Suggestion] R3-2:仍然成立——本轮文件有改动,但这三个测试仍未设置 DEPS_CACHE_ENV 就调用 provisionSourceOf。'rejects a marker naming a path inside the dependency root'（此处）、'rejects a marker naming a directory without the completeness marker'、'rejects a relative or empty recorded path' 都在函数第一个守卫（if (!cacheRoot) return null;）处返回,从不执行其名称和注释声称要钉住的安全守卫。紧邻上方的兄弟测试（'rejects a marker naming a valid entry OUTSIDE the configured cache'）用 withDepsCacheEnv 包裹调用,正是这三个测试省略的模式。
本文件的每次 vitest 运行中 QWEN_REVIEW_DEPS_CACHE 都未设置（仓库中没有任何地方为测试设置它）,因此删除三个生产守卫中的任何一个——if (insideDir(rootReal, real)) return null;、DEPS_COMPLETE_MARKER 的 existsSync、或 !recorded || !isAbsolute(recorded) 检查——整个套件仍然全绿。这些守卫正是阻止强提交的 .qwen-review-farm 标记把 scratch 树 containment 扩大到 PR 指定路径的防线。
修复:用 withDepsCacheEnv(<已存在的临时目录>, () => …) 包裹每个调用,让目标守卫成为决定者;完整性标记测试需把无标记目录放进缓存根内,使流程先通过缓存根 containment、只在缺标记处失败。包裹 env 后,删除每个守卫都必须让对应测试变红——现在一个都做不到。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| actual=$( | ||
| cd "$e" && find . -type f ! -name .qwen-review-deps-manifest ! -name .qwen-review-deps-complete | wc -l | ||
| ) | ||
| [ "$listed" -eq "$actual" ] |
There was a problem hiding this comment.
[Suggestion] R3-4: still stands — the file-count branch of entry_verifies ([ "$listed" -eq "$actual" ]) is the only check that catches a file ADDED to a sealed entry, and it is still never exercised by the shell suite: the two rebuild scenarios cover rev mismatch and content modification, not addition. (The TS consumer's mirror branch for a REMOVED file is a separate gap, deferred this round as D4-4; this entry is the shell side.)
A regression dropping or reordering the file-count comparison ships green: no scenario plants an extra file into a sealed entry, so a tamper that adds content (leaving manifest and marker untouched) passes the warm check and is served to every later review of any PR with an unchanged lockfile on this runner.
Add a rebuild scenario: seal an entry, plant one extra file inside it (manifest and marker untouched), and assert failed verification plus a repopulate. Removing the file-count comparison must turn it red.
中文说明
[Suggestion] R3-4:仍然成立——entry_verifies 的文件数分支（[ "$listed" -eq "$actual" ]）是唯一能捕获"向已密封条目添加文件"的检查,但 shell 套件仍未执行它:两个重建场景覆盖修订不匹配和内容修改,不覆盖添加。（TS 消费端针对"删除文件"的镜像分支是另一个缺口,本轮以 D4-4 推迟;本条是 shell 侧。）
删除或重排文件数比较的回归会全绿通过:没有场景向已密封条目植入额外文件,于是添加内容的篡改（manifest 与标记不动）通过热检查,并被服务给该 runner 上任何锁文件未变 PR 的后续每次评审。
修复:新增重建场景——密封条目后植入一个额外文件（不动 manifest 与标记）,断言 failed verification 与重新填充。删掉文件数比较,测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| snapshot "${path#./}" || { ok=false; break; } | ||
| done < <(find . \( -name .git -o -name node_modules \) -prune -o -type d -name dist ! -path ./dist -print -prune) | ||
| if [ "$ok" != true ]; then | ||
| echo '::warning::dependency-cache snapshot failed; discarding the stage' |
There was a problem hiding this comment.
[Suggestion] R3-7: still stands — three failure paths after a successful npm ci still exit 0 before the workspace cleanup at the end of the script: snapshot failure (here), seal failure, and the manifest mv failure. Each leaves the workspace holding the full ~2 GB install the population produced, for the next job's checkout clean and ownership repair to crawl.
A snapshot or seal failure (a disk hiccup, a torn hardlink) is exactly when the workspace is most likely to be partial and most expensive to re-walk; the job continues (fail-open by design) but pays nothing toward the strip the script itself wrote for this case.
Run the two workspace-strip finds before each of the three early exits (or route all three through a common cleanup-then-exit path). A scenario failing the snapshot and asserting the workspace's node_modules is gone pins the fix; removing the strip from that path turns it red.
中文说明
[Suggestion] R3-7:仍然成立——npm ci 成功后的三条失败路径仍在脚本末尾的 workspace 清理之前 exit 0:快照失败（此处）、密封失败、manifest mv 失败。每一条都把填充产出的约 2GB 完整安装留在 workspace 里,让下一个 job 的 checkout 清理与所有权修复去爬。
快照或密封失败（磁盘抖动、撕裂的硬链接）恰恰是 workspace 最可能残缺、重走最昂贵的时刻;job 继续（设计上失败开放）,却没有为脚本自己为此写好的清理付出任何东西。
修复:在三条提前退出之前运行两处 workspace 清扫 find（或让三者走共同的"先清理再退出"路径)。补场景:令快照失败,断言 workspace 的 node_modules 已被移除;从该路径删掉清扫,测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // root and nested node_modules, a member dist, and the root bundle | ||
| // output the snapshot must NOT take (regenerated per release, useless to | ||
| // a test run). | ||
| mkdirSync(join(work, '.git'), { recursive: true }); |
There was a problem hiding this comment.
[Suggestion] R3-9: still stands — the fixture plants a .git directory (here) but no assertion pins the -name .git -prune exclusions that keep it OUT of the published entry and the workspace strip; the cold-cache test asserts every other property of the entry (lockfile copy, npm marker, rev, manifest, nested node_modules, member dist, root-dist exclusion, dependency modes) except this one.
A future edit dropping -name .git -prune from either snapshot find ships green: the entry then carries the base checkout's .git — with it, every object of the default branch's history — into every worktree link-farmed from that entry, and the workspace strip likewise crawls it. No test notices.
In the cold-populate scenario, assert the published entry has no .git (expect(existsSync(join(r.entry, '.git'))).toBe(false)). Removing either -name .git -prune clause turns it red.
中文说明
[Suggestion] R3-9:仍然成立——fixture 种了 .git 目录（此处）,但没有任何断言钉住把 .git 挡在发布条目和 workspace 清扫之外的 -name .git -prune 排除子句;冷缓存测试断言了条目的所有其他属性（锁文件副本、npm 标记、rev、manifest、嵌套 node_modules、成员 dist、根 dist 排除、依赖权限位）,唯独缺这一项。
未来某次编辑从任一快照 find 中删掉 -name .git -prune 会全绿通过:条目随即带上 base checkout 的 .git——连同默认分支历史的全部对象——进入每个从该条目链接农场化的 worktree,workspace 清扫同样要爬它。没有测试会发现。
修复:冷填场景断言发布条目没有 .git（expect(existsSync(join(r.entry, '.git'))).toBe(false)）。删掉任一 -name .git -prune 子句,测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| expect(r.calls).not.toContain('npm'); | ||
| }); | ||
|
|
||
| it('the disk gate skips the install, never the job', () => { |
There was a problem hiding this comment.
[Suggestion] R3-10: still stands — the disk gate skips the install and exits 0 BEFORE the script's only reclamation legs: the entry prune and the torn-stage sweep both run after the successful publish the gate blocks, and no scenario exercises reclamation at all. The garbage that trips the gate persists, and the state that tripped the gate (low disk) guarantees the next run gates again — the orphans accumulate toward the very threshold that disables population.
Seed four entries (one prune-eligible) plus an aged torn stage, run under the disk gate, and assert the job continues AND reclamation ran — or explicitly decide the gate should skip reclamation and pin that decision. As written, the only mechanism that could recover disk space never runs in the one state that needs it.
中文说明
[Suggestion] R3-10:仍然成立——磁盘闸门跳过安装并在脚本仅有的两处回收腿之前 exit 0:条目剪枝与撕裂 stage 清扫都在闸门挡住的成功发布之后运行,且没有任何场景执行过回收。触发闸门的垃圾留存,而触发闸门的状态（磁盘低）保证下一次运行再次触发闸门——孤儿朝禁用填充的那个阈值不断累积。
修复:种四个条目（一个可被剪枝）加一个超龄撕裂 stage,在磁盘闸门下运行,断言 job 继续且回收已执行——或者明确决定闸门应跳过回收并把该决定钉住。现状是:唯一能回收磁盘空间的机制,恰恰在需要它的状态下从不运行。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
🔀 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 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-6 step-4b wiring (fetch-pr.ts:1020-1063) untested — already reported (comment 3868507385, open thread on fetch-pr.ts:1020); this round's probe confirmed the positions-1<->2 argument-swap mutant compiles and keeps all 156 fetch-pr tests g…
- R1-7 KEEP-IN-SYNC contract literals pinned only on the shell side — already reported (comment 3868507387, open thread on dep-provision.ts:109); rename mutation re-verified green this round
- provisionRoot nested-node_modules predicate has no test witness — already recorded in the round-4 deferral list (review 5049680835, worktree.ts:1613)
- behind-main PRs always fall back (recorded rev == merge base equality) — already recorded in the round-4 deferral list (review 5049680835, dep-provision.ts:254); documented fail-open design
- R1-4 graph.skipped members get no self-link/count — already reported (comment 3868507378, open thread on dep-provision.ts:325); this round's corroborating instances folded into the R4-3 re-post
- R1-12 warm-path QWEN_REVIEW_DEPS_CACHE export unpinned — already reported (comment 3868507406, open thread on qwen-pr-review-workflow.test.js)
- R1-15 stage-sweep -mmin +240 age threshold unpinned — already reported (comment 3868507417, open thread on qwen-pr-review-workflow.test.js)
Not reviewed: reverse audit — stopped at the 5-round cap without converging; rounds 1-5 each reported findings.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: platform test lanes — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped at this commit (merge_group/schedule triggers only); the PR's suites ran green locally on Linux only.
Not reviewed: the executable-script lint — .github/scripts/provision-review-deps.sh: shellcheck is not installed.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/dep-provision.ts:84 — [review] DEPS_MANIFEST_FILE doc overclaims the manifest as the writable-path guard, contradicting the script's trust-separation model (code-age rule: unchanged since the round-4 rev…scripts/tests/qwen-pr-review-workflow.test.js:4161 — [review] elevation test stubs chmod too, so the open R1-1 permission fix lands unwitnessed (code-age rule: unchanged since the round-4 reviewed head)
Convergence: round 5 posted 11 inline comment(s), 2 of them reported for the first time; the previous round posted 13 (8 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/dep-provision.ts (findings in round 4; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
[Critical] R5-1 provision-review-deps.sh:95-96 — an existing $ENTRY carrying no completeness marker is neither served nor dropped (the only drop_path "$ENTRY" lives inside the marker-gated block), and mv -T cannot rename over a non-empty directory, so a markerless foreign directory at the fully predictable entry name (sha256 of the public lockfile) permanently blocks publication for that lockfile; with <=2 lockfiles cached prune offers no self-heal. Witness: probe on the real script — no-sudo and sudo arms both warn 'could not move the staged dependency cache into place', run 2 repeats the full install (npm ci total 2); standalone mv -T over the non-empty dir fails ENOTEMPTY; the markerless-drop flip publishes and run 2 goes warm. (Dropped from inline: same line as the round-1/2 R1-1 marker-trust threads; not exempt as a re-post.)
[Critical] R4-2 dep-provision.ts:435 — still stands, re-measured at this commit: the member demand check compares a manifest range only against the TOP-LEVEL lock entry, never the member-nested entry where npm resolves the edge; on this repo's own lockfile 12 workspace members (incl. packages/cli, packages/core) fail the pass, so the farm falls back for EVERY PR and issue #10108 stays unfixed. Witness: probe sweep of the real provisionWorktreeDependencies over committed manifests+lockfile — provisioned:false failed:12, and the nested-lookup flip returns provisioned:true failed:0. Fix: prefer the member-nested lock key in the member pass, falling back to top-level. (Dropped from inline: same line as the round-4 R4-9 comment; the ledger carries this entry as R4-2.)
[Critical] R4-3 dep-provision.ts:604 — still stands, with new corroborating instances this round: the final gate only checks failed>0 || linked===0 and never reconciles the derived workspace graph against the members the lockfile records, so a silently-short farm yields provisioned:true plus the install-suppressing marker; the comment near memberDirs still asserts a linked-vs-expected check that does not exist. Measured instances: corrupt/deleted root package.json farms zero members yet stamps the marker (round-4 probe); graph.skipped members get nested install + dist but no self-link/demand/bin (this round, verified against real npm); a non-member file:/link: dep with dist/ passes demand then is skipped as 'self' (this round, flipping probe). Fix: reconcile link:true lockfile entries against self-linked members before the gate; tally.failed++ on unreadable root manifest and on graph.skipped. (Dropped from inline: same line as the round-4 R4-12 comment; the ledger carries this entry as R4-3.)
[Critical] R4-4 dep-provision.ts:664 — still stands: provisionSourceOf reads the PR-controlled farm marker with an unbounded, symlink-following readFileSync BEFORE any validation; a force-added symlink marker pointing at /dev/zero or a writerless FIFO crashes or hangs the review CLI (round-4 probe: FIFO arm timed out exit 124; /dev/zero arm died with an uncatchable std::bad_alloc). Fix: lstatSync the marker first and admit only a regular file of bounded size before reading. (Dropped from inline: same line as the round-4 R4-13 comment; the ledger carries this entry as R4-4.)
[Critical] R4-6 provision-review-deps.sh:232 (and :229) — still stands, ESCALATED from Suggestion to Critical by this round's probe: both torn-stage sweeps use bare rm -rf with no sudo leg (bypassing drop_path), so a root-owned stage left by an interrupted elevated publish can never be removed — a silent, permanent ~2GB disk leak per occurrence, 2>/dev/null suppressing the EACCES; the entry prune one line above routes root-owned paths through drop_path precisely because they need the sudo leg. Witness: plain find+rm -rf against an inaccessible dir exits 1, LEAK — stage survived. Fix: route both sweeps through drop_path. (Dropped from inline: same line as the round-4 R4-10 comment; the ledger carries this entry as R4-6.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 7 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without converging; rounds 1-5 each reported findings。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：platform test lanes — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped at this commit (merge_group/schedule triggers only); the PR's suites ran green locally on Linux only。
未审查：the executable-script lint — .github/scripts/provision-review-deps.sh: shellcheck is not installed。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 11 条行内评论，其中 2 条是首次提出；上一轮发布了 13 条（其中 8 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/dep-provision.ts（第 4 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R5-1 provision-review-deps.sh:95-96 — an existing $ENTRY carrying no completeness marker is neither served nor dropped (the only drop_path "$ENTRY" lives inside the marker-gated block), and mv -T cannot rename over a non-empty directory, so a markerless foreign directory at the fully predictable entry name (sha256 of the public lockfile) permanently blocks publication for that lockfile; with <=2 lockfiles cached prune offers no self-heal. Witness: probe on the real script — no-sudo and sudo arms both warn 'could not move the staged dependency cache into place', run 2 repeats the full install (npm ci total 2); standalone mv -T over the non-empty dir fails ENOTEMPTY; the markerless-drop flip publishes and run 2 goes warm. (Dropped from inline: same line as the round-1/2 R1-1 marker-trust threads; not exempt as a re-post.)
[Critical] R4-2 dep-provision.ts:435 — still stands, re-measured at this commit: the member demand check compares a manifest range only against the TOP-LEVEL lock entry, never the member-nested entry where npm resolves the edge; on this repo's own lockfile 12 workspace members (incl. packages/cli, packages/core) fail the pass, so the farm falls back for EVERY PR and issue #10108 stays unfixed. Witness: probe sweep of the real provisionWorktreeDependencies over committed manifests+lockfile — provisioned:false failed:12, and the nested-lookup flip returns provisioned:true failed:0. Fix: prefer the member-nested lock key in the member pass, falling back to top-level. (Dropped from inline: same line as the round-4 R4-9 comment; the ledger carries this entry as R4-2.)
[Critical] R4-3 dep-provision.ts:604 — still stands, with new corroborating instances this round: the final gate only checks failed>0 || linked===0 and never reconciles the derived workspace graph against the members the lockfile records, so a silently-short farm yields provisioned:true plus the install-suppressing marker; the comment near memberDirs still asserts a linked-vs-expected check that does not exist. Measured instances: corrupt/deleted root package.json farms zero members yet stamps the marker (round-4 probe); graph.skipped members get nested install + dist but no self-link/demand/bin (this round, verified against real npm); a non-member file:/link: dep with dist/ passes demand then is skipped as 'self' (this round, flipping probe). Fix: reconcile link:true lockfile entries against self-linked members before the gate; tally.failed++ on unreadable root manifest and on graph.skipped. (Dropped from inline: same line as the round-4 R4-12 comment; the ledger carries this entry as R4-3.)
[Critical] R4-4 dep-provision.ts:664 — still stands: provisionSourceOf reads the PR-controlled farm marker with an unbounded, symlink-following readFileSync BEFORE any validation; a force-added symlink marker pointing at /dev/zero or a writerless FIFO crashes or hangs the review CLI (round-4 probe: FIFO arm timed out exit 124; /dev/zero arm died with an uncatchable std::bad_alloc). Fix: lstatSync the marker first and admit only a regular file of bounded size before reading. (Dropped from inline: same line as the round-4 R4-13 comment; the ledger carries this entry as R4-4.)
[Critical] R4-6 provision-review-deps.sh:232 (and :229) — still stands, ESCALATED from Suggestion to Critical by this round's probe: both torn-stage sweeps use bare rm -rf with no sudo leg (bypassing drop_path), so a root-owned stage left by an interrupted elevated publish can never be removed — a silent, permanent ~2GB disk leak per occurrence, 2>/dev/null suppressing the EACCES; the entry prune one line above routes root-owned paths through drop_path precisely because they need the sudo leg. Witness: plain find+rm -rf against an inaccessible dir exits 1, LEAK — stage survived. Fix: route both sweeps through drop_path. (Dropped from inline: same line as the round-4 R4-10 comment; the ledger carries this entry as R4-6.)
— qwen3.8-max via Qwen Code /review (v0.22.2)
| find "$STAGE" -type f -exec chmod a-w {} + 2>/dev/null | ||
| if sudo -n chown -R root:root "$STAGE" 2>/dev/null && |
There was a problem hiding this comment.
[Critical] R1-1: still stands — the elevated publish leaves the published entry unreadable by the runner user, so the cache mechanism never works on exactly the pool members it exists for. The mktemp -d stage top directory is mode 0700 and chown -R root:root inherits it without any directory-mode widening (find -type f -exec chmod a-w touches files only), so the entry publishes as drwx------ root:root; additionally the manifest — mktemp'd at 0600 and only chmod a-w before elevation — publishes as 0400 root:root. Any passwordless-sudo pool member then fails end to end: fetch-pr's completeness-marker check hits EACCES and falls back with 'carries no completeness marker' on every run, the script's own warm check fails identically, and each run re-pays a full npm ci before failing to re-publish over the occupied unreadable entry — a directory-mode-only fix still dies at entryManifestValid's readFileSync of the 0400 manifest, so both instances need fixing together.
witness: probe on the unmodified script — ENTRY-TOP: mode=700 owner=… drwx------ after the elevated publish; with a chmod go+rx widening inserted the entry publishes 755 and the consumer provisions; manifest mode trace: mktemp 600 → chmod a-w → 400 → chown root:root.
Open the stage's directory modes and the manifest's read bits before elevation:
| find "$STAGE" -type f -exec chmod a-w {} + 2>/dev/null | |
| if sudo -n chown -R root:root "$STAGE" 2>/dev/null && | |
| # Immutable files BEFORE the elevation (the user still owns the stage), | |
| # so a degraded re-publish never serves writable payloads either. | |
| find "$STAGE" -exec chmod a+rX,u-w,go-w {} + 2>/dev/null | |
| if sudo -n chown -R root:root "$STAGE" 2>/dev/null && |
The elevation scenario in scripts/tests/qwen-pr-review-workflow.test.js must additionally assert the published entry's top dir grants non-owner read+execute and the manifest is group/other-readable (with the shim executing chmod for real); removing either mode grant must turn it red.
中文说明
[Critical] R1-1：仍然成立——提权发布把发布出的条目变得对 runner 用户不可读,于是缓存机制恰恰在它为之存在的池成员上从不工作。mktemp -d 的 stage 顶层目录是 0700,chown -R root:root 原样继承而从未放开目录权限位（find -type f -exec chmod a-w 只处理文件）,条目发布为 drwx------ root:root;此外 manifest——由 mktemp 以 0600 创建、提权前仅 chmod a-w——发布为 0400 root:root。任何免密 sudo 池成员都会端到端失败:fetch-pr 的完整性标记检查因 EACCES 回退为 'carries no completeness marker',脚本自身的 warm 检查同样失败,每次运行都重跑完整 npm ci 然后因无法覆盖被占用的不可读条目而再次发布失败——仅放开目录位的修复仍会死在 entryManifestValid 对 0400 manifest 的 readFileSync 上,两个实例必须一起修。
见证:对未修改脚本的探针——提权发布后 ENTRY-TOP: mode=700 drwx------;插入 chmod go+rx 放开后条目以 755 发布且消费端供给成功;manifest 权限轨迹:mktemp 600 → chmod a-w → 400 → chown root:root。
修复:提权前放开 stage 目录模式与 manifest 读位（见 suggestion）。elevation 场景还需断言发布条目顶层目录对非属主可读可执行、manifest 对组/其他可读（shim 需真实执行 chmod）;删掉任一权限放开,测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| # and the trailing `exit 0` makes that total. | ||
| set -u | ||
| set +e | ||
| CACHE_ROOT="$HOME/.qwen-review-deps" |
There was a problem hiding this comment.
[Critical] R4-1: still stands — every privileged operation in this script escalates THROUGH $CACHE_ROOT without ever re-verifying the root's own identity, and the root sits in the user-writable $HOME. rename(2) checks write permission on the PARENT, not ownership of the directory, so unsandboxed PR code on the same persistent host can mv ~/.qwen-review-deps ~/.qwen-review-deps.old && mkdir ~/.qwen-review-deps, then plant an entry for the public base lockfile hash with a self-consistent manifest, a source-rev at the current main tip, the completeness marker, and poisoned node_modules. The next review's warm check and entry_verifies pass attacker-authored state, and fetch-pr's farm (lockfile byte-compare, entryManifestValid, source-rev gate) passes self-authored state — probes execute attacker code inside a job holding pull-requests: write, persistently.
witness: round-4 on-host execution of the chain (unprivileged rename of the 755 root:root root succeeded; planted entry served warm; payload intact); round-5 re-derivation confirms no ownership check exists anywhere in the script at this commit.
Re-establish the root's identity at the top of every run before any trust or escalation — require $CACHE_ROOT to be a real directory and, on sudo members, root-owned (stat -c %u = 0); on failure move the occupant aside (rename via the parent needs no sudo) and mkdir -p a fresh root. Longer-term, root the cache under a directory whose parent the runner user cannot write.
Add a workflow-suite case that renames the cache root aside and re-plants a user-owned self-consistent entry, asserting the step does NOT print 'dependency cache warm' and rebuilds instead; removing the ownership gate turns it red.
中文说明
[Critical] R4-1：仍然成立——脚本中所有提权操作都"穿过" $CACHE_ROOT 执行,却从不重新验证根目录自身的身份,而根目录位于用户可写的 $HOME 中。rename(2) 检查的是父目录的写权限而非目录属主,因此同一持久主机上未加沙箱的 PR 代码可以 mv ~/.qwen-review-deps ~/.qwen-review-deps.old && mkdir ~/.qwen-review-deps,再为公开 base 锁文件哈希植入自洽 manifest、指向当前 main tip 的 source-rev、完整性标记与投毒 node_modules 的条目。下一次评审的 warm 检查与 entry_verifies 都通过攻击者自造的状态,fetch-pr 的农场化（锁文件字节比对、entryManifestValid、source-rev 闸门）也通过自造状态——探针在持有 pull-requests: write 的 job 里执行攻击者代码,且持久复现。
见证:第 4 轮在本机执行完整链条（非特权重命名 755 root:root 根目录成功;植入条目被热路径供给;载荷完好）;第 5 轮重新推导确认本提交的脚本中任何位置都没有所有权检查。
修复:每次运行开始、任何信任或提权之前重建根目录身份——要求 $CACHE_ROOT 是真实目录,sudo 成员上要求 root 所有（stat -c %u = 0）;失败则把占用者经父目录挪走（无需 sudo）并 mkdir -p 新根。长期方案:把缓存放到 runner 用户不可写的父目录之下。
补一个 workflow 场景:重命名缓存根并植入用户所有的自洽条目,断言步骤不输出 'dependency cache warm' 而是重建;删掉所有权闸门,测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const demanded = new Set<string>([ | ||
| ...Object.keys(manifest.dependencies ?? {}), | ||
| ...Object.keys(manifest.devDependencies ?? {}), | ||
| ...Object.keys(manifest.peerDependencies ?? {}), | ||
| ]); |
There was a problem hiding this comment.
[Critical] R5-2: the demand check also never reads overrides, which npm ci's sync check validates — same class as the recorded optionalDependencies hole, different field. A PR that edits overrides in package.json (e.g. forces a transitive dep to a different major) without regenerating package-lock.json passes the demand pass, the farm places npm's .package-lock.json marker, and build-test skips the npm ci that would have rejected the desync loudly — probes then run against the pre-override resolved versions and any version-dependent failure is attributed to the diff instead of the desync. This repo's root package.json really carries overrides (line 100), so the trigger field exists here.
witness: npm 10.9.8 A/B — a lockfile for left-pad@1.3.0 plus overrides: {"left-pad": "1.0.0"} added without regenerating → npm ci prints npm error code EUSAGE, exit 1; the in-sync control exits 0. The farm-side trace admits the same shape, and the marker gate at npm-toolchain.ts:741-742 then skips the install.
| const demanded = new Set<string>([ | |
| ...Object.keys(manifest.dependencies ?? {}), | |
| ...Object.keys(manifest.devDependencies ?? {}), | |
| ...Object.keys(manifest.peerDependencies ?? {}), | |
| ]); | |
| const demanded = new Set<string>([ | |
| ...Object.keys(manifest.dependencies ?? {}), | |
| ...Object.keys(manifest.devDependencies ?? {}), | |
| ...Object.keys(manifest.peerDependencies ?? {}), | |
| ]); | |
| // npm's sync check validates overrides too (EUSAGE on desync); a manifest | |
| // whose overrides disagree with the lockfile root entry is demand-unmet. | |
| if ( | |
| manifest.overrides !== undefined && | |
| JSON.stringify(manifest.overrides) !== | |
| JSON.stringify(lockPackages?.['']?.overrides) | |
| ) { | |
| return true; | |
| } |
(Note: MemberManifest also needs an overrides?: Record<string, string> field, and this check belongs in demandUnmet where lockPackages is in scope — the suggestion block shows the shape, not the exact placement.) A dep-provision.test.ts case with a manifest carrying overrides not matching the lockfile root entry must return provisioned:false; deleting the check makes it red.
中文说明
[Critical] R5-2：需求检查同样从不读取 overrides,而 npm ci 的同步检查会校验它——与已记录的 optionalDependencies 缺口同类,不同字段。PR 修改 package.json 的 overrides（如把某传递依赖强制到另一个 major）却不重新生成 package-lock.json 时,需求检查通过,农场放置 npm 的 .package-lock.json 标记,build-test 跳过本会大声拒绝该失配的 npm ci——探针跑在 override 之前解析的版本上,任何版本相关的失败都被记到 diff 头上而非失配本身。本仓库根 package.json 确实带有 overrides（第 100 行）,触发字段真实存在。
见证:npm 10.9.8 A/B——left-pad@1.3.0 的锁文件加上未重生成锁文件的 overrides: {"left-pad": "1.0.0"} → npm ci 输出 npm error code EUSAGE、退出码 1;同步的对照退出 0。农场侧追踪确认同一形态被接纳,而 npm-toolchain.ts:741-742 的标记闸门随后跳过安装。
修复:把 overrides 失配判为需求未满足（MemberManifest 需要增加 overrides?: Record<string, string> 字段,检查放在能拿到 lockPackages 的 demandUnmet 内;suggestion 块示意形态而非精确位置）。补测试:清单带有与锁文件根条目不一致的 overrides → provisioned:false;删掉检查,测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // Presence-only for non-semver specs (file:, git+, links) and for | ||
| // lockfiles that record no version — the check npm reduces those to. |
There was a problem hiding this comment.
[Suggestion] R4-5: still stands — the demand check degrades to presence-only for every non-semver spec (file:, git+, links), but lockfile v3 records a checkable identity for those specs too (resolved: "../core" for file:/link: deps, the full git URL for git+), so a desynced file: target passes the farm: a PR repointing a member's file: dependency at another target without regenerating the lockfile gets provisioned:true and the marker suppresses the npm ci that would have caught it, and the wrong target is what resolves in probes.
Compare the lockfile's recorded identity for non-semver specs (the resolved field for file:/link:, the git URL for git+) against the manifest's spec instead of reducing to presence, and add a case whose file: spec's lockfile resolved target diverges, expecting provisioned:false; removing the identity comparison turns it red.
中文说明
[Suggestion] R4-5：仍然成立——需求检查对所有非 semver 规格（file:、git+、links）退化为仅查存在性,但 lockfile v3 对这些规格同样记录了可校验的身份（file:/link: 依赖的 resolved: "../core"、git+ 的完整 git URL）,因此失配的 file: 目标能通过农场:PR 把某成员的 file: 依赖改指向另一目标却不重新生成锁文件时,得到 provisioned:true,标记压掉本可发现失配的 npm ci,探针里解析到的是错误目标。
修复:对非 semver 规格改用锁文件记录的身份（file:/link: 的 resolved 字段、git+ 的 git URL）与清单规格比对,而不是退化为存在性;补一个 file: 规格的 resolved 目标偏离的用例,期望 provisioned:false;删掉身份比对,测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if [ -w "$CACHE_ROOT" ]; then | ||
| STAGE="$(mktemp -d "$CACHE_ROOT/.stage.XXXXXX")" || exit 0 |
There was a problem hiding this comment.
[Suggestion] R4-7: still stands — no scenario in the deps-cache suite ever exercises the [ ! -w "$CACHE_ROOT" ] staging branch (stage under $HOME), which is the steady-state shape on sudo members after the first elevated publish, and the harness has no homeStages observation, so the branch and its torn-stage cleanup are not even assertable. On the real pool, run 1 publishes a root-owned cache root and every later lockfile-bump re-population flows through this untested branch (mktemp under $HOME + seal + sudo -n mv -T); if it is broken the cache silently never re-populates after a lockfile bump — reviews fall back to the in-budget npm ci that #10108 exists to remove, with the suite green.
Add a homeStages observation to runDepsCacheStep and a scenario whose prepare makes the cache root non-writable (e.g. chmodSync(cacheRoot, 0o555)), asserting the stage lands under $HOME and the publish succeeds, plus a warm-path variant asserting the sudo -n touch freshen against a root-owned entry. Removing the [ -w ] guard or the $HOME-stage fallback must turn the new scenario red.
中文说明
[Suggestion] R4-7：仍然成立——依赖缓存套件从未执行 [ ! -w "$CACHE_ROOT" ] 的 stage 分支（stage 落在 $HOME 下）,而这是 sudo 成员首次提权发布后的稳态形态;且 harness 没有 homeStages 观测,该分支及其残留 stage 清理根本无法断言。真实池上,第 1 次运行发布 root 所有的缓存根,之后每次锁文件变更的重新填充都走这条未测分支（$HOME 下 mktemp + 密封 + sudo -n mv -T）;若它有缺陷,锁文件变更后缓存会静默地再也不重新填充——评审回退到 #10108 要消灭的预算内 npm ci,而套件全绿。
修复:给 runDepsCacheStep 增加 homeStages 观测,并加一个 prepare 把缓存根设为不可写（如 chmodSync(cacheRoot, 0o555)）的场景,断言 stage 落在 $HOME 下且发布成功;另加一个 warm 变体断言对 root 所有条目的 sudo -n touch 刷新。删掉 [ -w ] 闸门或 $HOME-stage 回退,新场景必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| [ -n "$recorded" ] && [ "$recorded" = "$current" ] || return 1 | ||
| [ -f "$e/.qwen-review-deps-manifest" ] || return 1 | ||
| ( cd "$e" && sha256sum -c --quiet .qwen-review-deps-manifest >/dev/null 2>&1 ) || return 1 | ||
| listed=$(wc -l < "$e/.qwen-review-deps-manifest") |
There was a problem hiding this comment.
[Suggestion] R3-4: still stands, with a new twist verified by probe this round — the file-count branch of entry_verifies ([ "$listed" -eq "$actual" ], the only check that catches a file ADDED to a sealed entry) is still never exercised by the shell suite, AND its ! -name exclusion matches basenames at ANY depth. Executed: deleting the count comparison keeps 14/14 dependency-cache scenarios green; against the real script, planting node_modules/dep-a/.qwen-review-deps-complete yields 'dependency cache warm' (bypass) where an ordinary planted file yields 'failed verification'. The end-to-end farm harm is blocked at this commit by dep-provision.ts's exact-set entryManifestValid (probe: reserved-name-at-depth → provisioned:false 'fails its content manifest'), so the residue is a test gap plus a bounded ops effect on no-sudo members — a reserved-name plant keeps a poisoned entry reporting warm while every fetch-pr falls back per-review.
Add an added-file tamper scenario (pins the tripwire) and a reserved-name-at-depth scenario, and anchor the exclusion to the entry root (explicit root-level paths or -maxdepth 1 scope) instead of basename ! -name. The added-file scenario must go red when the count comparison is removed.
中文说明
[Suggestion] R3-4：仍然成立,本轮探针还验证出一个新变体——entry_verifies 的文件计数分支（[ "$listed" -eq "$actual" ],唯一能抓住向已密封条目新增文件的检查）依旧没有任何 shell 场景执行,且其 ! -name 排除在任意深度按文件名匹配。已执行:删掉计数比较,14/14 依赖缓存场景全绿;对真实脚本植入 node_modules/dep-a/.qwen-review-deps-complete 得到 'dependency cache warm'（绕过）,而植入普通文件得到 'failed verification'。端到端的农场化危害在本提交被 dep-provision.ts 的精确集合 entryManifestValid 挡住（探针:深层保留名 → provisioned:false 'fails its content manifest'）,因此残留是测试缺口加上无 sudo 成员上的有界运维影响——保留名植入会让投毒条目持续报告 warm,而每个 fetch-pr 逐次回退。
修复:补一个新增文件篡改场景（钉死绊线）和一个深层保留名场景,并把排除锚定到条目根（显式根级路径或 -maxdepth 1 范围）而非按文件名的 ! -name。删掉计数比较时,新增文件场景必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if [ "$ok" != true ]; then | ||
| echo '::warning::dependency-cache snapshot failed; discarding the stage' | ||
| drop_path "$STAGE" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
[Suggestion] R3-7: still stands, re-verified against the script at this commit — three failure paths after a successful npm ci (snapshot failure here, seal failure below, and the manifest mv failure) exit 0 before the workspace cleanup at the end of the script, each leaving the workspace holding the full node_modules + built dist the cleanup exists to strip. Every such run hands the next job's checkout and ownership repair a ~2GB tree to crawl — exactly the cost the trailing strip removes on the success path.
Run the workspace cleanup (the two find | rm loops) on these failure paths too, or move the cleanup ahead of the populate-or-fail section; a workflow-suite scenario failing the snapshot must then assert the workspace's node_modules/dist are gone afterwards — today they persist.
中文说明
[Suggestion] R3-7：仍然成立,已在本提交的脚本上重新核对——npm ci 成功之后的三条失败路径（此处的快照失败、下方的密封失败、manifest mv 失败）都在脚本末尾的 workspace 清理之前 exit 0,每一条都把完整 node_modules + 构建产物留在 workspace 里,而清理正是为了移除它们。每一次这样的运行都让下一个 job 的 checkout 与所有权修复去爬一棵 ~2GB 的树——恰是成功路径上末尾清理省掉的成本。
修复:在这些失败路径上也执行 workspace 清理（两个 find | rm 循环）,或把清理移到 populate-or-fail 段之前;随后补一个使快照失败的 workflow 场景,断言 workspace 的 node_modules/dist 已被移除——今天它们仍在。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // root and nested node_modules, a member dist, and the root bundle | ||
| // output the snapshot must NOT take (regenerated per release, useless to | ||
| // a test run). | ||
| mkdirSync(join(work, '.git'), { recursive: true }); |
There was a problem hiding this comment.
[Suggestion] R3-9: still stands — the fixture plants a .git directory (here) but no assertion pins the -name .git -prune exclusions that keep it OUT of the published entry and the workspace strip; the cold-cache test asserts every other property of the entry except this one. Removing either -name .git -prune (snapshot or strip) keeps the whole suite green while the entry starts carrying .git objects — and the strip starts deleting the workspace's .git mid-review — so the exclusion that protects both is pinned by nothing.
In the cold-cache scenario, assert the published entry contains no .git path and the workspace's .git survives the strip; the new assertions must go red when either -name .git -prune is removed from the script.
中文说明
[Suggestion] R3-9：仍然成立——fixture 在这里植入了 .git 目录,但没有任何断言钉死把 .git 排除在发布条目与 workspace 清理之外的 -name .git -prune;冷缓存测试断言了条目的其他一切属性,唯独没有这一项。删掉任一 -name .git -prune（快照或清理）整个套件仍全绿,而条目会开始携带 .git 对象、清理会开始在评审中途删除 workspace 的 .git——保护两者的排除条件没有任何钉住。
修复:在冷缓存场景中断言发布的条目不含任何 .git 路径、workspace 的 .git 在清理后仍在;删掉脚本中任一 -name .git -prune 时,新断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| expect(r.calls).not.toContain('npm'); | ||
| }); | ||
|
|
||
| it('the disk gate skips the install, never the job', () => { |
There was a problem hiding this comment.
[Suggestion] R3-10: still stands — the disk gate skips the install and exits 0 BEFORE the script's only reclamation legs: the entry prune and the torn-stage sweeps both run after the successful publish the gate blocks, and no scenario exercises reclamation under the gate. If reclamation were reordered before the gate, a low-disk run would prune healthy entries and sweep live stages while doing nothing else — the exact data loss the gate's 'never the job' contract is supposed to bound — with every test green.
Extend this scenario to pre-seed a stale entry plus an old torn stage and assert both SURVIVE the gated run; the extended scenario must go red if the prune/sweep move above the disk gate.
中文说明
[Suggestion] R3-10：仍然成立——磁盘闸门跳过安装并在脚本仅有的两条回收路径之前退出 0:条目剪枝与残留 stage 清扫都运行在闸门所阻断的成功发布之后,且没有任何场景在闸门触发时验证回收。若回收被重排到闸门之前,低磁盘运行会在什么都不安装的同时剪掉健康条目、扫掉存活 stage——正是闸门 '绝不挂 job' 契约应当兜住的数据丢失——而所有测试全绿。
修复:扩展本场景,预先放置一个过期条目和一个旧的残留 stage,断言两者在闸门触发后仍在;把剪枝/清扫移到磁盘闸门之上时,扩展场景必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const inEntry = existsSync( | ||
| join(entryReal, 'node_modules', ...dep.split('/')), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R5-3: a file:/link: dependency whose target is not a workspace member passes the demand presence check here (existsSync follows the entry symlink, and semver.validRange of the file: spec is null so the range arm is skipped) but is silently dropped by the farm: farmFromEntry's verdict classifies every entry symlink resolving inside the entry but outside a node_modules as a workspace self-link and skips it, and only workspace-graph members are re-derived — so the worktree gets provisioned: true and the install-suppressing marker with no node_modules/<dep> link at all, and probes importing the dep die MODULE_NOT_FOUND in a tree the plan report claims fully provisioned, attributed to the PR under review. The hole is confined to the partially-materialised case (a non-member file: target carrying a dist/ subtree, which the snapshot guarantees) and is unreachable at this commit — a sweep of the committed lockfile finds 25 link: true entries, none non-member — but it breaks the module's own stated invariant the first day someone vendors a package outside the workspaces globs.
witness: probe — arm A (dist subtree): {provisioned: true, failed: 0, worktreeFooLinkExists: false, npmMarkerPlaced: true}; arm B (dangling target): provisioned:false (correct fallback); the fail-closed flip makes arm A provisioned:false.
Re-derive non-member link deps (for each lockfile link: true entry that is not a workspace member, create node_modules/<name> → the worktree's resolved target), or minimally treat a file:/link: spec as demand-unmet unless the self-link loop re-derives it, so the farm falls back to the disclosed npm ci path. A dep-provision.test.ts case whose entry carries node_modules/foo → non-workspace vendor/foo with dist/ must assert provisioned:false (fail-closed) or a worktree node_modules/foo link (re-derive); removing the guard restores today's silent skip and the assertion reds.
中文说明
[Suggestion] R5-3：目标不是 workspace 成员的 file:/link: 依赖能通过此处的需求存在性检查（existsSync 跟随条目符号链接,且 file: 规格的 semver.validRange 为 null 使范围分支被跳过）,但会被农场静默丢弃:farmFromEntry 的 verdict 把条目内、所有 node_modules 之外解析的条目符号链接都归为 workspace self-link 并跳过,而只有 workspace 图成员会被重新推导——于是 worktree 得到 provisioned: true 和抑制安装的标记,却完全没有 node_modules/<dep> 链接,探针导入该依赖时在一棵计划报告声称完整供给的树里 MODULE_NOT_FOUND,被记到受审 PR 头上。该缺口局限于部分物化形态（带 dist/ 子树的非成员 file: 目标,快照必然物化它）,且在本提交不可达——对已提交锁文件的扫描得到 25 个 link: true 条目,无一非成员——但一旦有人把包 vendor 到 workspaces 通配之外,它就打破了模块自己声明的不变量。
见证:探针——臂 A（带 dist 子树）:{provisioned: true, failed: 0, worktreeFooLinkExists: false, npmMarkerPlaced: true};臂 B（悬空目标）:provisioned:false（正确回退）;失败即回退的翻转使臂 A 变为 provisioned:false。
修复:为非成员 link 依赖重新推导（对每个非 workspace 成员的锁文件 link: true 条目,创建 node_modules/<name> → worktree 内解析的目标）,最低限度则把 file:/link: 规格视为需求未满足,除非 self-link 循环会重新推导它,使农场回退到披露式 npm ci 路径。补一个条目携带 node_modules/foo → 非 workspace vendor/foo（带 dist/）的测试,断言 provisioned:false（失败即回退）或 worktree 存在 node_modules/foo 链接（重新推导）;删掉守卫即恢复今天的静默跳过,断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
3 participants
What this PR does
On the persistent review runners, the worktree a PR review builds now gets its dependencies provisioned from a host-level cache instead of needing an install. A workflow step keeps a per-lockfile cache entry populated — a real
npm ciresult with every workspace's built output, snapshotted by hardlink and published atomically with a completeness marker written last — and the fetch that builds the review worktree link-farms the entry matching the PR's own lockfile into it, in seconds. The outcome is recorded in the plan report as data (dependencies), the bundled skill documents the new field and tightens the "never install by hand" rule for provisioned trees, and the scratch-tree isolation learns the provisioned shape so per-verifier trees keep getting their dependency farm instead of counting every borrowed package as an escape.Three semantics are deliberate. Workspace self-links are re-derived against the worktree's own members, never mirrored from the cache — mirrored, a probe of code importing a changed sibling would quietly test the base's copy of it. Prebuilt sibling
distis copied, never linked, so the scoped rebuild the build agent already performs overwrites the worktree and cannot write through into the shared cache. And npm's completeness marker is placed only on a farm that completed with nothing dropped, so the build agent's install gate stays honest: a partial farm stays markerless and the old install path repairs it.Everything fails open toward today's behaviour: a cold cache, a PR that changes the lockfile, a worktree that commits its own
node_modules, or a farm that could not complete each records a reason and the review runs exactly as before — the disclosed read-only round is the pre-cache status quo, never a regression.Why it's needed
Issue #10108: the review worktree has no
node_modulesand no built workspace outputs, and a full install plus prerequisite builds does not fit inside an agent's tool budget. So every probe or chunk agent that decided the right evidence was "run the test" failed the same way, burned its budget on a doomed install, and the round downgraded to a read-only audit with a "Not explored to full depth (tool budget reached)" disclosure — observed on PR #9729 rounds 13 and 15, and again on PR #9940's own review, which could not run the very tests that PR adds. Probes settle verdicts that re-reading cannot; this blunted exactly the strongest verification tool the skill has. The CI reviews run on the persistent ECS pool, where a host-level cache is feasible and cheap; the missing piece was wiring it into the worktree the review builds.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/lib/dep-provision.test.ts src/commands/review/lib/worktree.test.ts— the provisioner's 16 cases (cold cache, diverged lockfile, torn snapshot, committednode_modules/distrefusals, dangling self-links, escaping links, hostile member names, forged provenance markers) plus the two new farm-over-a-provisioned-tree cases.npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/qwen-pr-review-workflow.test.js -t 'dependency-cache'— 10 cases driving the step's real bash with a stubbed npm/df: cold populate keyed by lockfile hash with the root bundle excluded, warm path installing nothing, failed install degrading with the export intact, disk gate skipping the install and never the job, LRU prune sparing live stage dirs, the env/marker literals the CLI reads, and the step's ordering and pool gate.dependency cache cold for lockfile <hash>; installing (once per lockfile per runner, outside every agent's budget)ordependency cache warm, and the review's fetch stderr reportsDependencies provisioned from <entry>: N packages linked …. A build-and-test agent on a provisioned worktree skips its install; probe agents can run tests immediately. A PR that modifiespackage-lock.jsoninstead reportsDependencies NOT provisioned (no cache entry for lockfile …)and behaves exactly as today.Evidence (Before & After)
Before: PR #9729 round 13 — "could not execute
session-pr-refresh.test.ts— the review worktree lacks built workspace-package dist outputs"; PR #9940's review could not run its own new tests ("nonode_modulesin the review worktree").After (measured end-to-end on a real npm workspace fixture, real registry install, the step's bash extracted verbatim from the YAML): the step populated the cache entry atomically and stripped the workspace; provisioning a fresh bare worktree reported
provisioned: truewith 3 packages linked, 2 workspace self-links, 1 dist copy;require()through the farm resolved a member importing its sibling plus a third-party dependency; overwriting the worktree's member build output flipped the nextrequire()immediately while the cache's copy stayed byte-identical (self-links point at the worktree, the copy protects the cache); npm's completeness marker was in place; a scratch tree farmed from the provisioned worktree reported zero failures, and removing the provenance marker made the same call count the cache-resolving link as an escape again — the containment extension is load-bearing in both directions. The disk gate also fired live once (the dev box has ~4G free, under the 10G threshold) and the job continued with the export intact.Tested on
Environment (optional)
Unit and workflow-script tests via vitest; end-to-end via a real
npm ciagainst the public registry, the workflow step's extracted bash, and Node resolution through the provisioned worktree. actionlint + shellcheck clean on the workflow (the pre-existingenvironment.deploymentnote aside). Three test failures reproduce identically on unmodified main under a root test environment (permission-bit-based cases root ignores) and are unrelated.Risk & Scope
continue-on-error; the run that pays it also leaves the workspace stripped so the next job's checkout stays cheap. Cache entries are ~2G each, pruned to the newest three. Provisioned siblingdistis stale for packages the PR changes until the build agent's scoped rebuild overwrites it — the same rebuild it performs today, now against a tree whose install is already done.node_modulesor memberdistin the PR is refused or left alone rather than farmed over; entry links that escape the cache are counted and withheld, and a farm with any failure withholds npm's completeness marker so the install gate re-installs.Linked Issues
Fixes #10108
中文说明
这个 PR 做了什么
在持久评审 runner 上,PR 评审构建的 worktree 现在从宿主级缓存供给依赖,而不再需要安装。一个 workflow 步骤维护按锁文件哈希键控的缓存条目——真实
npm ci的结果加上每个 workspace 的构建产物,用硬链接快照、原子发布、完整性标记最后落盘——而构建评审 worktree 的抓取过程把与 PR 自己的锁文件匹配的条目用链接农场映进 worktree,只需数秒。结果作为数据(dependencies)记录在 plan report 里,内置 skill 文档化了新字段并对已供给的树收紧了"绝不手动安装"的规则;scratch-tree 隔离也学会了已供给的形态,per-verifier 树照常获得依赖农场,而不是把每个借来的包都判成逃逸。三个语义是刻意设计的:workspace self-link 按 worktree 自己的成员重新推导,绝不镜像缓存的——若镜像,探针测试导入了被改动兄弟包的代码时,会悄悄测到 base 的副本;预构建的兄弟
dist用复制而非链接,构建 agent 本就要做的定向重建覆盖的是 worktree,写不穿共享缓存;npm 的完整性标记只在零失败的农场上放置,保证构建 agent 的安装闸门诚实——部分失败的农场不带标记,由旧的安装路径修复。一切都朝今天的行为回退:冷缓存、PR 改了锁文件、worktree 自带 committed
node_modules、农场未能完成——每种情况都记录原因,评审照旧运行;披露式只读轮次本来就是无缓存时代的现状,绝不是回归。为什么需要
Issue #10108:评审 worktree 没有
node_modules也没有构建产物,完整安装加前置构建放不进单个 agent 的工具预算。于是每个判断"正确证据是把测试跑起来"的 probe/chunk agent 都以同样方式失败,预算烧在注定失败的安装上,轮次降级为只读审计并披露 "Not explored to full depth (tool budget reached)"——PR #9729 第 13、15 轮实测,PR #9940 自己的评审再次命中(连该 PR 新增的测试都跑不了)。probe 能裁决重读裁决不了的问题;这钝化的恰是这个 skill 最强的验证手段。CI 评审跑在持久 ECS 池上,宿主级缓存可行且廉价;缺的只是把它接进评审构建的 worktree。评审测试计划
如何验证
cd packages/cli && npx vitest run src/commands/review/lib/dep-provision.test.ts src/commands/review/lib/worktree.test.ts—— 供给器 16 个用例(冷缓存、锁文件偏离、撕裂快照、committednode_modules/dist拒绝、悬空 self-link、逃逸链接、敌意成员名、伪造 provenance 标记)加上"在已供给的树上再建农场"的两个新用例。npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/qwen-pr-review-workflow.test.js -t 'dependency-cache'—— 10 个用例用 stub npm/df 驱动步骤的真实 bash:按锁文件哈希键控的冷路径填充(排除根 bundle)、热路径零安装、安装失败但导出保留、磁盘闸只跳过安装不挂 job、LRU 剪枝不碰存活 stage 目录、CLI 读取的环境变量/标记字面量、步骤顺序与池门控。dependency cache cold …; installing或dependency cache warm,抓取的 stderr 报告Dependencies provisioned from <entry>: N packages linked …。已供给 worktree 上的构建测试 agent 跳过安装;probe agent 可立即跑测试。改了package-lock.json的 PR 则报告Dependencies NOT provisioned (…),行为与今天完全一致。证据(Before & After)
Before:PR #9729 第 13 轮——"could not execute
session-pr-refresh.test.ts— the review worktree lacks built workspace-package dist outputs";PR #9940 的评审跑不了自己新增的测试("nonode_modulesin the review worktree")。After(在真实 npm workspace fixture 上端到端实测,真实 registry 安装,步骤 bash 从 YAML 原样提取):步骤原子填充缓存条目并清理 workspace;对全新裸 worktree 供给报告
provisioned: true,3 个包链接、2 个 self-link、1 个 dist 复制;穿过农场的require()解析了导入兄弟包和第三方依赖的成员;覆写 worktree 的成员构建产物后下一次require()立即生效而缓存副本逐字节不变(self-link 指向 worktree,复制保护缓存);npm 完整性标记在位;从已供给 worktree 建 scratch 农场零失败,移走 provenance 标记后同一调用把解析进缓存的链接重新判为逃逸——containment 扩展双向承重。磁盘闸也真实触发过一次(开发机仅 ~4G 空闲,低于 10G 阈值),job 继续且导出保留。测试平台
风险与范围
continue-on-error约束;付费的那次运行同时清理 workspace,让下一个 job 的 checkout 保持廉价。条目约 2G,保留最新三个。PR 改动的包其供给 dist 在构建 agent 定向重建前是陈旧的——那正是它今天就在做的重建,只是树的安装已经完成。node_modules或成员dist被拒绝或原样保留;逃出缓存的条目链接被计数并拒绝镜像;任何失败的农场不放 npm 完整性标记,安装闸门会重新安装。关联 Issue
Fixes #10108