feat(review): run the reviewed repository's own commands behind a container (#9556) - #9723
Conversation
…tainer (#9556) A review executes the code it is reviewing. `build-test` runs whatever the reviewed repository's `package.json` names — `npm ci` with its `preinstall` and `postinstall` scripts, the build, the suite — and `test-efficacy` runs that suite again once per baseline, control, mutant, hunk probe and revert. Both did it as the invoking identity, and both handed the PR's code `process.env` entire: on CI that carries `OPENAI_API_KEY` and `GH_TOKEN`. Reading them is one line in a `postinstall`, and it needs none of the git-config machinery the pipeline's threat findings are built on. The boundary goes around the executions, not around the review agent. Wrapping the agent was tried first and is the wrong shape: its secrets do not survive the container's env allowlist, its `timeout` reaps the host-side client rather than the container, its CLI version stops matching the runner's — and after all of that the mount is the whole checkout, so `<repo>/.git` stays writable anyway. Three decisions the argv encodes, each measured rather than assumed: - **The mount is the review temp dir, not the tree the command runs in.** The dependency farm links OUT of every tree — `exposeDependencies` points each package in the probe tree's `node_modules` at the review worktree's copy, 1 722 of them on a live CI review. Mounting one tree would leave every link dangling. Every tree the pipeline builds is a sibling under `.qwen/tmp`, so one mount covers both ends while `<repo>/.git` stays outside it. - **The environment is an allowlist**, not the inherited one. - **The network is per command kind.** An install needs the registry; a build and a suite do not, and `--network none` keeps loopback so a suite that stands up a local fixture server still runs. One ephemeral container per command. A long-lived one per phase would save about 1–2% of the 540-second efficacy budget and would re-introduce exactly the cross-run state #9221 spent rounds closing. Off by default: containerising a build by surprise changes what native modules compile against. `review.sandbox` is `off` | `auto` | `required`, read through `operatorReviewSettings` — which skips the workspace scope, so a repository cannot ship a `.qwen/settings.json` that switches off the containment existing to contain it. `QWEN_REVIEW_SANDBOX` outranks it so CI can require containment without depending on a settings file the runner may not carry. Each of the three decisions is pinned by a test that goes red when that decision alone is reverted.
|
Re-run after 22 commits since the first pass; the branch merged
Moving on to code review. 🔍 中文说明首轮之后又落了 22 个提交，分支在
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI wrote down my own design before reading the diff: operator-scope policy setting (
I also checked the base-side dependencies the module leans on: The two open Criticals from the review loop (R1-3, R19-1). I verified both premises at the base commit rather than taking either side's word: the worktree leases live at CI evidence at
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ✅ success |
| Test (macos-latest, Node 22.x) | ⏭️ skipped |
| Test (windows-latest, Node 22.x) | ⏭️ skipped |
| Integration Tests (CLI, No Sandbox) | ⏭️ skipped |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ✅ success |
| Desktop Shell (ubuntu-22.04) | ✅ success |
| Desktop Shell (windows-2022) | ✅ success |
| Dependency CVE audit | ✅ success |
| Secret scan (TruffleHog) | ✅ success |
| Classify PR | ✅ success |
| review-pr (bot orchestration) | 🔄 in progress |
Sandboxed verification would settle the one claim static review cannot: @qwen-code /verify — that the pinned argv actually starts a container against a real runtime, that the farm's ~1 700 links resolve through the mount, and that verdicts match the direct path. The author's machine has no container runtime, so every behavioural statement about the contained path rests on the pinned argv and the author's account of earlier live measurements; the suite itself exercises only the default off path. Not verified here: real-runtime behaviour (no runtime available to this review, and this run never executes PR code).
中文说明
代码审查
读 diff 之前我先写下了自己的设计：操作者作用域的策略设置（off/auto/required，默认 off）、每命令一个一次性 --rm 容器、挂载 review 临时目录以便依赖 farm 向外的链接可解析、白名单环境替代继承环境、仅 install 给网络。PR 与此一致，并在要紧处走得更远：
- 按来源而非按名单处理 env。
isFileSourcedEnvKey让仓库自带的.env无法决定任何本应由操作者 shell 决定的事——环境只能收紧策略、不能放松；来自被审仓库的QWEN_REVIEW_SANDBOX_IMAGE被直接忽略，因为镜像就是代码本身。已核实 loader 只在真实环境本无该键时才记为文件来源，所以擦除是精确还原而非近似。 required现在真的会拒绝。 首轮的主要阻断已消除：refuseUnsandboxedPhase在三个阶段（build-test、test-efficacy、test-delta）的顶部被消费，包括 spawn 门禁看不见的 agent shell 移交路线；--resume上的拒绝是 throw 而不是覆盖在途报告。不可挂载的树在required下拒绝、在auto下回退——两个方向的契约都写对了。- 探针套件改用镜像 PATH 上的
node，不再是宿主机的process.execPath——首轮预言的 ENOENT 已关闭，vitest 入口与探针路径经shellQuotePath加引号。 - 复用：test-delta 里那份小心复制的
run()被删除，改用共享实现——这也是 base 侧重跑与 PR 侧跨越同一边界的原因，delta 的两半不会再形状漂移。
另核实了模块依赖的基线侧设施：shell-quote、REVIEW_TMP_DIR、redirectedAncestor、CUSTOM_SANDBOX_IMAGE_ENV_VAR 与文件来源追踪集合都存在于父提交，且语义与注释所述一致。
审查循环遗留的两条 Critical（R1-3、R19-1）。 两边说法我都没有直接采信，而是在基线提交上亲自核实：工作树 lease 确实落在 join(repositoryRoot, REVIEW_TMP_DIR)，cleanupReviewWorktreeLeases 确实只按会话标识匹配、无来源校验；而 main 上，被审仓库的命令今天就以宿主用户身份、带着完整环境、在毫无文件系统限制的情况下运行。所以 gitfile 与 lease 这两类资产在本 PR 存在之前就已可从任何一次审查中够到——容器化把可达面收窄到一个挂载，而不是扩大。它们是真问题，但不是本 PR 引入的回归；修复（把宿主可信状态移出可写挂载）属于几何改动，PR 描述已明确将其另行跟踪。该修复必须先于还是后于本 PR 落地，是维护者的范围决策，不是门禁的决策——已在反思部分点名。
5d2a126 上的 CI 证据
所有 pull_request 事件的工作流运行均已完成：Qwen Code CI — 成功、Security Checks — 成功。仍在运行的 review-pr 检查是机器人自己的审查作业，不属于本 PR 的 CI。macOS/Windows 单测与 CLI 集成本提交上跳过；承载 review 测试的是 ubuntu 套件。表格见英文部分。
沙箱验证可以了结静态审查看不到的唯一论断：@qwen-code /verify——钉住的 argv 能否在真实运行时上真正起容器、farm 的约 1700 条链接能否经由挂载解析、判定是否与直接路径一致。作者机器上没有容器运行时，所以关于容器化路径的一切行为论断都建立在钉住的 argv 与作者对早前实测的转述之上；套件本身只跑默认 off 路径。本次未验证：真实运行时行为（本审查无运行时可用，且本运行从不执行 PR 代码）。
— Qwen Code · qwen3.8-max
Reviewed at 5d2a126de0109fcce82908c2cbf365e1f6f00c4b · re-run with @qwen-code /triage
|
Confidence: 3/5 — the code is ready; the reason this doesn't end in an approval is a human decision this gate is not allowed to make, not a defect in the diff. Stepping back: the branch that arrived is not the one I sent back in the first pass. All three things that kept it at 2/5 are fixed and pinned — the schema is regenerated and CI has been green since, What keeps this at 3/5 is honesty about what I can and cannot attest. The default ⏸️ Deferring to @pomelo-nwu / @yiliang114 — two calls need a maintainer, and neither can be made from the diff:
No approval and no request-changes from me this round: the branch is green, the earlier blockers are closed, and the remaining questions are policy, not code. 中文说明置信度：3/5 —— 代码已经就绪；本轮不以批准收尾，是因为摆着一个本门禁无权代答的人类决策，而不是 diff 里还有什么缺陷。 退一步看：抵达这里的分支已不是首轮被我退回的那个。首轮压在 2/5 上的三件事都已修好且被测试钉住：schema 已重新生成、CI 自此常绿； 停在 3/5 是对自己能证明与不能证明之事的诚实。默认 ⏸️ 转交 @pomelo-nwu / @yiliang114 —— 有两个决定需要维护者来做，且都无法从 diff 中得出：
本轮我不给批准、也不请求修改：分支是绿的，早先的阻断项已关闭，遗留问题是策略问题，不是代码问题。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs some rethinking — see my notes above. The short version: regenerate packages/vscode-ide-companion/schemas/settings.schema.json (CI is red on this commit), and either enforce the refused verdict at the two call sites or hold the required option until that lands — as it stands, required silently runs the reviewed code unsandboxed when no runtime answers. 🙏
The generated JSON Schema is checked in and CI diffs it against a fresh run (`npm run generate:settings-schema`). Adding `review.sandbox` to `settingsSchema.ts` without regenerating left the two out of step, which is what the "settings.schema.json is out of date" gate is for. The second red check, `Post Coverage Comment`, failed at "Download coverage reports artifact" — a consequence of the test job dying before it uploaded one, not an independent failure.
…t/review-sandbox-ci
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-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): 324 passed — this review observed 22989, 495 passed.
中文说明
Test Plan（非阻断）：324 passed — this review observed 22989, 495 passed。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const verdict = sandboxVerdict(); | ||
| if (verdict.kind !== 'container') return null; |
There was a problem hiding this comment.
[Critical] The required policy fails open: the refusal this diff promises has no enforcement point anywhere, and the reviewed code runs unsandboxed with the full environment in four demonstrated shapes. Both call sites test only verdict.kind !== 'container' and return null, falling through to the direct spawn with buildRunEnv() — the entire process.env. sandboxVerdict() produces a refused verdict when required meets no answering runtime, but nothing consumes it; the doc comment delegates refusal to "the caller's decision", and no caller ever decides. Three further paths fail open even once that is fixed: the nesting guard returns direct when SANDBOX is set BEFORE policy is consulted — so required inside qwen --sandbox runs with the full inherited environment (the seatbelt spawn inherits the entire process.env), and the PR's own test pins this bypass; a cwd outside .qwen/tmp — the documented local-checkout /review mode — hits at < 0 and returns null even though the verdict IS container; and the npm-toolchain hand-off (yarn/pnpm/bun repos, no lockfile — "the common case" per its own comment) tells the agent to run the PR's install/build/test itself (agent-briefs.ts:540 wires exactly that), never consulting the policy even when a runtime answers. Measured: with QWEN_REVIEW_SANDBOX=required and no runtime answering, a probe drove the real run() — the verdict was {"kind":"refused"}, yet the command executed and a probe secret placed in the review process's env appeared in the PR command's output; a one-line refusal-consumption patch flips the probe to a thrown refusal. Local-checkout arm: real run() with a stub docker and an outside cwd → exit 0, output SECRET-IS:hunter2-credential, docker log shows only docker info. Hand-off arm: yarn.lock fixture under required vs off → reports byte-identical, zero contained executions; a policy-consulting patch flips it. This is the unresolved point of the existing CHANGES_REQUESTED review, and it still stands at this head. Either thread the verdict through to the callers so required produces an evidence-unavailable result instead of spawning/instructing (all four paths), or hold the required option out of the schema until that enforcement exists.
中文说明
required 策略失守时不吭声：本 diff 承诺的拒绝在代码里没有任何执行点，四种已实测的形态下被审代码都会带着完整环境在沙箱外运行。两个调用点都只判 verdict.kind !== 'container' 就返回 null，落入带 buildRunEnv()（整个 process.env）的直接 spawn。required 且无运行时时 sandboxVerdict() 确实产出 refused 判定，但没有任何代码消费它；注释把拒绝推给"调用者的决定"，而没有任何调用者做这个决定。即使修掉这一条，还有三条路径照样失守：嵌套守卫在 SANDBOX 被设置时先于策略返回 direct——在 qwen --sandbox 内部设 required 会带着完整继承环境直跑（seatbelt spawn 继承整个 process.env），本 PR 自己的测试钉住了这个绕过；cwd 不在 .qwen/tmp 之内——即文档化的本地检出 /review 模式——即使判定是 container 也会命中 at < 0 返回 null；npm-toolchain 的交接路径（yarn/pnpm/bun 仓库、无 lockfile——按其注释是"常见情况"）指示 agent 自行运行 PR 的 install/build/test（agent-briefs.ts:540 正是这样接线的），全程不查询策略，即使有运行时也照跑。实测：QWEN_REVIEW_SANDBOX=required 且无运行时时，探针驱动真实 run()——判定为 {"kind":"refused"}，命令却照样执行，放进审查进程环境的探针密钥出现在 PR 命令输出里；一行消费拒绝的补丁即可翻转为抛错拒绝。本地检出臂：stub docker + 外部 cwd 下真实 run() → exit 0，输出 SECRET-IS:hunter2-credential，docker 日志只有 docker info。交接臂：required 与 off 下 yarn.lock 夹具报告逐字节相同、容器化执行次数为零；查询策略的补丁可翻转。这正是既有 CHANGES_REQUESTED 审查中未解决的点，在本 head 上依旧成立。要么把判定贯通到调用者、让 required 产出"证据不可用"而不是 spawn/指示（四条路径都要），要么在强制落地前先从 schema 里拿掉 required 选项。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const suite = `${shellQuotePath(process.execPath)} ${shellQuotePath( | ||
| findVitestBin(dependencyRoot), | ||
| )} run --reporter=json ${probes.map(shellQuotePath).join(' ')}`; |
There was a problem hiding this comment.
[Critical] The sandboxed probe suite bakes the host's Node path into the container command, and that path does not exist inside the container — every sandboxed probe run exits 127, so test-efficacy yields zero evidence exactly when containment is on. containerCommand delivers this string as sh -lc '<suite>', but the only mount is the review temp dir and the sandbox image's Node lives at its own path (/usr/local/bin/node in the node:22-slim lineage); /usr/bin/node (here) or /opt/hostedtoolcache/… (GitHub runners) is neither mounted nor present in the image. build-test's npm commands resolve through the container's PATH and are unaffected — this is the only command that hardcodes the host path. Downstream, exit 127 with empty stdout maps every probe — baseline, control, each mutant, hunk probes, revert — to inconclusive/no-output, blaming the runner's output for a sandbox-wiring error. Measured against a live daemon with the module's exact argv shape: sh -lc '/usr/bin/node --version' → sh: 1: /usr/bin/node: not found (exit 127), while the control arm (node via the image PATH) in the same container exits 0 printing v22.23.2. Use the image's own toolchain for the boxed branch — node resolves on the image's PATH:
const suite = `node ${shellQuotePath(findVitestBin(dependencyRoot))} run --reporter=json ${probes.map(shellQuotePath).join(' ')}`;
keeping process.execPath for the direct spawn. (This was point 3 of the earlier triage review — still open at this head.)
中文说明
沙箱化探针套件把宿主机的 Node 路径嵌进容器命令，而该路径在容器内不存在——每个沙箱化探针运行都以 127 退出，恰好在开启 containment 时整个 test-efficacy 阶段零证据。containerCommand 以 sh -lc '<suite>' 传递该字符串，但唯一挂载是 review 临时目录，镜像的 Node 在自己的路径（node:22-slim 谱系的 /usr/local/bin/node）；/usr/bin/node（本机）或 /opt/hostedtoolcache/…（GitHub runner）既不在挂载里也不在镜像里。build-test 的 npm 命令经容器 PATH 解析、不受影响——只有这条命令硬编码宿主路径。下游 127 + 空 stdout 会把所有探针（基线、对照、每个突变体、hunk 探针、回退）判为 inconclusive/no-output，把沙箱接线错误归咎于运行器输出。对真实守护进程、按模块的 argv 形状实测：sh -lc '/usr/bin/node --version' → sh: 1: /usr/bin/node: not found（exit 127）；同一容器内走镜像 PATH 的对照臂正常输出 v22.23.2（exit 0）。容器分支改用镜像自带工具链（node 走镜像 PATH），process.execPath 只留给直接 spawn。（这是先前审查意见的第 3 点，在本 head 上依旧成立。）
— qwen3.8-max via Qwen Code /review (v0.21.15)
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] The RW mount covers every pipeline tree's .git gitfile — containerized PR code can rewrite a gitfile to redirect later HOST-side git invocations at a planted repository, and obtain host code execution through filter.*. All pipeline trees (review worktree, -probe, -base, -scratch-*) are linked worktrees under <repo>/.qwen/tmp, and each tree's .git gitfile sits inside the tree — i.e. inside this RW mount. Code running as root in the container rewrites <tree>/.git to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge = <payload> and a .gitattributes applying it. Two host-side git consumers then follow the redirect: probe-tree creation (git worktree add --detach <probeTree> <sha> with the review worktree as cwd — neither the gitfile verification nor the localFilterCommands gate that scratch-tree.ts has), and restoreProbeTreeTracked after every sandboxed suite run — its backpointer round-trip PASSES because the attacker writes the very gitdir file the check reads, and its inert -c overrides neutralize core.hooksPath/core.fsmonitor but NOT filter.*, so its checkout --force HEAD -- . materializes planted blobs through the planted smudge filter. The module doc's "<repo>/.git stays outside it" is true of the common dir and false of the security property: the per-tree gitfiles that select which gitdir host git consults stay writable inside the mount. Reproduced end-to-end on git 2.43.0 in throwaway trees mirroring the layout: the check sequence passed (backpointer round-trip read the attacker-planted gitdir), checkout exit 0, HOST-SIDE EXECUTION OBSERVED for both the restore checkout and git worktree add; scratch-tree's --git-common-dir identity gate refused the same tree. Treat the per-tree gitfile as attack surface: after each sandboxed run verify every tree's .git still resolves to its own admin entry under <repo>/.git/worktrees/ (outside the mount) and refuse on mismatch; extend scratch-tree's localFilterCommands refusal and gitfile check to probe-tree creation and restoreProbeTreeTracked.
中文说明
RW 挂载覆盖了每棵流水线树的 .git gitfile——容器内的 PR 代码可以改写 gitfile，把之后的宿主侧 git 重定向到挂载内植入的仓库，并借 filter.* 获得宿主代码执行。所有流水线树（review 工作树、-probe、-base、-scratch-*）都是 <repo>/.qwen/tmp 下的链接工作树，各自的 .git gitfile 位于树内——即在此 RW 挂载之内。容器内以 root 运行的代码把 <tree>/.git 改写为 gitdir: <同一挂载下植入的目录>，其中带 filter.<x>.smudge = <payload> 与应用它的 .gitattributes。两个宿主侧 git 消费者会跟随重定向：探针树创建（以 review 工作树为 cwd 的 git worktree add --detach <probeTree> <sha>——既无 gitfile 校验、也无 localFilterCommands 关卡，而 scratch-tree.ts 两者都有）；以及每次沙箱化套件运行后的 restoreProbeTreeTracked——其回指针往返检查会通过，因为攻击者写下的正是该检查读取的 gitdir 文件；其惰性 -c 覆盖能中和 core.hooksPath/core.fsmonitor，却中和不了 filter.*，于是 checkout --force HEAD -- . 会经植入的 smudge filter 物化植入内容。模块文档的"<repo>/.git 留在挂载外"对 common dir 为真，对安全性质为假：决定宿主 git 查哪个 gitdir 的每棵树的 gitfile 仍在挂载内可写。已在 git 2.43.0 上按该布局的一次性树中端到端复现：检查序列通过（回指针往返读到攻击者植入的 gitdir）、checkout exit 0、restore checkout 与 git worktree add 均观测到宿主侧执行；scratch-tree 的 --git-common-dir 身份关卡对同一棵树会拒绝。把每棵树的 gitfile 当作攻击面：每次沙箱化运行后校验各树 .git 仍解析到 <repo>/.git/worktrees/（挂载外）下自己的 admin entry，不一致即拒绝；把 scratch-tree 的 localFilterCommands 拒绝与 gitfile 检查扩展到探针树创建与 restoreProbeTreeTracked。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const marker = `${sep}${REVIEW_TMP_DIR}${sep}`; | ||
| const at = resolved.indexOf(marker); |
There was a problem hiding this comment.
[Critical] The mount-root marker search uses first-occurrence indexOf: when the checkout itself sits under a .qwen/tmp, the mount widens to the OUTER temp dir and pulls <repo>/.git and every sibling tree into the container — defeating this module's stated property that <repo>/.git stays outside. A review invoked from inside another review's worktree — this pipeline's own dogfood geometry — is exactly such a nested layout. Executing the verbatim slice arithmetic: for probeTree = /srv/.qwen/tmp/checkouts/myrepo/.qwen/tmp/review-pr-1-probe, indexOf yields tmpDir = /srv/.qwen/tmp (mounting the entire outer checkout family), while lastIndexOf yields /srv/.qwen/tmp/checkouts/myrepo/.qwen/tmp (the intended dir); lastIndexOf returned the intended dir on all three layouts executed (normal, nested, and this machine's actual review path). The flat case computes correctly, which is why the new tests pass while this corner stays open; the identical copy in build-test's containerised has the same shape. Tree names cannot contain separators (scratch labels flatten to [A-Za-z0-9._-]), so the deepest occurrence is always the tree's true parent temp dir, and the at < 0 guard still rejects non-temp-dir trees:
| const marker = `${sep}${REVIEW_TMP_DIR}${sep}`; | |
| const at = resolved.indexOf(marker); | |
| const at = resolved.lastIndexOf(marker); |
中文说明
挂载根的标记查找用了首次出现的 indexOf：当检出目录本身位于某个 .qwen/tmp 之下时，挂载会扩到外层临时目录，把 <repo>/.git 与所有兄弟树一起拉进容器——与本模块声明的"<repo>/.git 留在挂载外"相悖。从另一个 review 工作树内部发起的审查——本流水线自身的 dogfood 几何——正是这种嵌套布局。按原样执行切片算术：对 probeTree = /srv/.qwen/tmp/checkouts/myrepo/.qwen/tmp/review-pr-1-probe，indexOf 得到 tmpDir = /srv/.qwen/tmp（挂进整个外层检出家族），lastIndexOf 得到 /srv/.qwen/tmp/checkouts/myrepo/.qwen/tmp（预期目录）；在三种布局（常规、嵌套、本机真实 review 路径）上 lastIndexOf 均返回预期目录。扁平情形计算正确，这正是新测试通过而该角落仍敞开的原因；build-test 的 containerised 里的同款副本同病。树名不可能含分隔符（scratch 标签被压平为 [A-Za-z0-9._-]），因此最深一次出现总是该树真正的父临时目录，at < 0 守卫仍会拒绝非临时目录树。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, | ||
| '--workdir', | ||
| opts.cwd, |
There was a problem hiding this comment.
[Critical] No UID/GID handling: under rootful docker the container runs as uid 0 and writes root-owned files into the host-mounted trees, which the non-root host pipeline then cannot modify or delete. The argv carries no --user, and the default image (repo Dockerfile: FROM node:22-slim, no USER directive) runs as root; containerized npm ci/build/suite then create root-owned node_modules and build output inside the mounted trees, and later host-side steps — the install-timeout rmSync of node_modules, discardWorktree, cleanup sweeps, scratch-tree resets — hit EACCES, leaving residue that accumulates across reviews: the cross-run-state class #9221 closed. utils/sandbox.ts maps host UID/GID (SANDBOX_SET_UID_GID / shouldUseCurrentUserInSandbox) for exactly this hazard on the same image lineage, and #9556's risk section names UID/GID as a condition of this design. Observed live during verification: argv has no --user flag; container uid 0 vs host uid 1000; files created by the container read back uid 0; host-side rmSync(recursive) on the container-created dirs failed EACCES: permission denied — cleanup afterwards needed a root-privileged container to remove the residue. Add '--user', \${process.getuid()}:${process.getgid()}`(guarded onprocess.getuid` existing), mirroring sandbox.ts and its opt-out.
中文说明
没有 UID/GID 处理：rootful docker 下容器以 uid 0 运行，会在宿主挂载的树里写入 root 属主文件，宿主侧非 root 流水线随后既改不了也删不掉。argv 没有 --user，默认镜像（仓库 Dockerfile：FROM node:22-slim，无 USER 指令）以 root 运行；沙箱化的 npm ci/构建/套件于是在挂载树内创建 root 属主的 node_modules 与构建产物，之后的宿主步骤——安装超时时 rmSync 删 node_modules、discardWorktree、清理扫描、scratch-tree 复位——会撞 EACCES，留下跨审查累积的残留：即 #9221 关闭过的那类跨运行状态。utils/sandbox.ts 对同一镜像谱系正是为此映射宿主 UID/GID（SANDBOX_SET_UID_GID/shouldUseCurrentUserInSandbox），#9556 的风险小节也把 UID/GID 列为本设计的条件之一。验证期间实测：argv 无 --user 标志；容器 uid 0 对宿主 uid 1000；容器创建的文件读回 uid 0；宿主侧对容器所建目录的 rmSync(recursive) 报 EACCES: permission denied——事后清理需要 root 权限容器才能删掉残留。加 '--user', \${process.getuid()}:${process.getgid()}`（以 process.getuid` 存在为守卫），与 sandbox.ts 及其豁免开关保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const marker = `${sep}${REVIEW_TMP_DIR}${sep}`; | ||
| const at = resolved.indexOf(marker); | ||
| if (at < 0) return null; | ||
| const tmpDir = resolved.slice(0, at + marker.length - 1); |
There was a problem hiding this comment.
[Suggestion] The single RW mount gives every containerized command write access to ALL sibling trees and pipeline artifacts — including the merge-base baseline the A/B comparison trusts. The mount's WIDTH is a documented decision (the farm's links dangle otherwise) — write access to every other tree is a different claim and is never named as an accepted cost. Everything lives under this one dir: the -base baseline tree, -probe trees, -scratch-* trees, lease files, report JSON. A containerized npm ci (kind install, network on) with a committed postinstall can enumerate the mount and patch or delete sources in the -base tree; the later test-delta comparison then reads PR-introduced failures back as pre-existing in the corrupted baseline — netNew comes back empty and a real regression is masked. Observed live with the module's exact mount spec: from a container workdir'd in review-pr-9, the sibling trees and lease files were visible; a write into the -base tree succeeded (WRITE-OK; read back CORRUPTED-BY-PR-POSTINSTALL, uid=0). base-tree.ts builds the baseline via runBuildTest in baseWorktreePath(worktree), confirming the consumer chain. Keep the tmpDir mount for link resolution, but overlay the sibling trees read-only (--volume <sibling>:<sibling>:ro — nested bind mounts are honoured over the wider one), leaving .npm-cache and the command's own tree RW.
中文说明
单一 RW 挂载让每个容器化命令都能写入所有兄弟树与流水线产物——包括 A/B 对比所信任的 merge-base 基线。挂载的"宽度"是有文档记载的决定（否则 farm 链接悬空）——但对其他所有树的写权限是另一个命题，且从未被列为接受的代价。一切都在这一个目录下：-base 基线树、-probe 树、-scratch-* 树、租约文件、报告 JSON。带提交 postinstall 的容器化 npm ci（install 类别、有网络）可以枚举挂载并改写或删除 -base 树里的源码；之后的 test-delta 对比会把 PR 引入的失败从被污染的基线里读回为"本就存在"——netNew 为空，真实回归被掩盖。按模块的挂载参数实测：从工作目录在 review-pr-9 的容器里可见兄弟树与租约文件；对 -base 树的写入成功（WRITE-OK；读回 CORRUPTED-BY-PR-POSTINSTALL，uid=0）。base-tree.ts 经 baseWorktreePath(worktree) 里的 runBuildTest 构建基线，确认了消费链。保留 tmpDir 挂载以解析链接，但把兄弟树以只读叠加挂载（--volume <sibling>:<sibling>:ro——嵌套绑定挂载优先于更宽的挂载生效），.npm-cache 与命令自身的树保持 RW。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const suite = `${shellQuotePath(process.execPath)} ${shellQuotePath( | ||
| findVitestBin(dependencyRoot), | ||
| )} run --reporter=json ${probes.map(shellQuotePath).join(' ')}`; |
There was a problem hiding this comment.
[Suggestion] The composed sh -lc suite command is untested — removing ALL quoting from it changes no test result. This sandbox branch is the first place probe file paths reach a shell: the direct arm passes an argv array (no shell); the boxed arm interpolates each path into one sh -lc string via shellQuotePath. This diff makes that quoting load-bearing for correct execution, but nothing exercises the composed command — shellQuotePath's escape behavior is pinned only through agent-prompt prompt-string assertions, not through anything that executes it. A quoting regression — or a probe path containing a space or an apostrophe (a checkout under ~/Documents/John's Projects/…) — is misparsed inside the container: the wrong file runs or a probe is silently skipped, and only when sandboxing is on. Mutation: stripping every shellQuotePath call from the composed string → the five relevant test files still pass 283/283; comparator: defanging the apostrophe escape flips two agent-prompt tests — the suite-composition path has no such pin. Add a test asserting the composed suite round-trips to the intended argv for a probe path containing a space and an apostrophe (or a colocated shell-quote test driving the boxed branch).
中文说明
组装出的 sh -lc 套件命令没有测试——把其中所有引号处理都删掉，也没有任何测试结果变化。这个沙箱分支是探针文件路径第一次进入 shell：直接臂传 argv 数组（无 shell）；容器臂经 shellQuotePath 把每个路径插进一条 sh -lc 字符串。本 diff 使该引号处理成为正确执行的承重件，但没有任何测试驱动这条组装命令——shellQuotePath 的转义行为只被 agent-prompt 的提示词断言钉住，没有被任何会执行它的东西钉住。引号回归——或含空格/撇号的探针路径（检出在 ~/Documents/John's Projects/… 之类）——会在容器内被错误解析：跑错文件或静默跳过探针，且只在开启沙箱时发生。变异测试：剥掉组装字符串中全部 shellQuotePath 调用 → 相关五个测试文件仍 283/283 全过；对照：弄坏撇号转义会让两个 agent-prompt 测试变红——套件组装路径没有这样的钉子。补一个测试：断言含空格与撇号的探针路径经组装后往返出预期 argv（或放一个驱动容器分支的 shell-quote 同目录测试）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| typeof review?.severityFloor === 'string' | ||
| ? review.severityFloor | ||
| : undefined, | ||
| sandbox: typeof review?.sandbox === 'string' ? review.sandbox : undefined, |
There was a problem hiding this comment.
[Suggestion] review.sandbox — this feature's on/off switch — has no test at either the loader or the schema layer. review-settings.test.ts covers effort/severityFloor/reverseAuditRounds/attribution/comment passthrough and defaults, but never asserts sandbox passthrough; settingsSchema.test.ts has no assertion for the new enum, and the dialog-membership test pins five review.* keys but not review.sandbox despite showInDialog: true. The policy-layer tests in sandboxed-exec.test.ts all inject {sandbox: …} directly, bypassing operatorReviewSettings() — so a loader or schema regression silently changes whether containment engages, and no test fails. Mutations in a scratch tree: loader reading the wrong key (sandboxMode) → 23/23 pass; schema default off→auto + showInDialog false → 55/55 pass; the comparator (mutating a tested setting) fails its test — the harness can detect this class; review.sandbox simply has no coverage. Add cases mirroring the existing effort tests: 'required' passes through raw, a non-string drops to undefined, and the schema exposes the enum with default off.
中文说明
review.sandbox——本功能的总开关——在加载层与 schema 层都没有测试。review-settings.test.ts 覆盖了 effort/severityFloor/reverseAuditRounds/attribution/comment 的透传与默认值，却从未断言 sandbox 透传；settingsSchema.test.ts 对新枚举没有任何断言；对话框成员测试钉了五个 review.* 键，唯独不含 review.sandbox（尽管它 showInDialog: true）。sandboxed-exec.test.ts 里的策略层测试全部直接注入 {sandbox: …}，绕过了 operatorReviewSettings()——加载层或 schema 的回归会静默改变 containment 是否生效，而没有任何测试失败。在 scratch 树中变异：加载层读错键（sandboxMode）→ 23/23 全过；schema 默认值 off→auto＋showInDialog false → 55/55 全过；对照项（变异一个有测试的设置项）会令测试失败——测试框架能发现这类问题，只是 review.sandbox 没有覆盖。参照现有 effort 测试补用例：'required' 原样透传、非字符串落为 undefined、schema 暴露该枚举且默认 off。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| /** | ||
| * The container argv for one probe-suite run, or null to spawn it directly. |
There was a problem hiding this comment.
[Suggestion] Orphaned doc comment: probeContainer was inserted between restoreProbeTreeTracked's doc and the function itself. The long comment ending "Refusing is the only answer that is neither." documented restoreProbeTreeTracked's non-obvious refuse-when-.git-absent rationale; JSDoc binds to the immediately-following declaration, so after this insertion it reads as a preamble to probeContainer, while the function whose rationale it carries loses its documentation. (The same defect class at build-test's containerised is reported separately.) Move probeContainer (with its own comment) above the "Put the probe tree's TRACKED files back…" block, or after restoreProbeTreeTracked.
中文说明
孤立的文档注释：probeContainer 被插在 restoreProbeTreeTracked 的文档注释与该函数之间。以"Refusing is the only answer that is neither."结尾的长注释记录的是 restoreProbeTreeTracked 在 .git 缺失时拒绝的非显而易见之理；JSDoc 绑定紧随其后的声明，插入之后这段注释读起来成了 probeContainer 的前言，而真正需要它的函数失去了文档。（同类缺陷也出现在 build-test 的 containerised，另行报告。）把 probeContainer（连同其注释）移到"Put the probe tree's TRACKED files back…"块之上，或 restoreProbeTreeTracked 之后。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| /** | ||
| * The container argv for one reviewed-repository command, or null to run it |
There was a problem hiding this comment.
[Suggestion] Orphaned doc comment (second site): containerised was inserted directly after run()'s pre-existing doc comment. The comment above this block — "Exported for the one thing an injected exec cannot cover: that the failing set is measured HERE, off the raw text, and survives a trim…" — documented run(); JSDoc binds to the immediately-following declaration, so run() now silently loses the rationale for its raw-text failingFiles contract, while this new function gains a floating comment that is factually wrong for it (it is not exported and has nothing to do with exec or FAIL lines). A future edit to run()'s trim/parse seam loses the one comment explaining why the parse must read the raw text — the exact divergence that comment was written to prevent. (Mirror of the test-efficacy orphan, reported separately.) Move containerised (with its comment) above the run() doc block, or after run().
中文说明
孤立的文档注释（第二处）：containerised 被插在 run() 既有文档注释的正下方。此块上方的注释——"导出是为了那件注入的 exec 覆盖不了的事：失败集合在这里基于原始文本测量，并能在裁掉 FAIL 行后存活……"——记录的是 run()；JSDoc 绑定紧随其后的声明，于是 run() 悄悄失去了其原始文本 failingFiles 契约的理据，而这个新函数得到一段对它而言事实错误的悬空注释（它并未导出，也与 exec/FAIL 行无关）。未来对 run() 裁剪/解析接缝的修改会失去唯一解释"为何必须解析原始文本"的注释——那正是该注释要防止的偏离。（与另行报告的 test-efficacy 处为镜像缺陷。）把 containerised（连同注释）移到 run() 的文档块之上，或 run() 之后。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…s the first cut shipped Five Criticals from the review, all of them real, and the first two would have made the feature not work at all. - **`required` failed open.** `sandboxVerdict` produced a `refused` verdict and nothing consumed it: both call sites tested `kind !== 'container'` and fell through to the direct spawn with the full environment. Refusal is now decided ONCE, at the top of each phase, before anything executes — which is also the only place that can cover the route that never reaches a spawn: a repo whose toolchain cannot be scoped is handed to the AGENT's own shell, and a gate at the spawn would leave that wide open under the very policy forbidding it. - **`SANDBOX` was a shortcut past the policy.** The first cut returned `direct` when the session was already sandboxed, reasoning that the outer boundary is the one the operator asked for. Wrong for this property: the CLI's own sandbox constrains the filesystem and hands the child `process.env` entire, and stripping the secrets is half of what `required` promises. - **The probe suite baked in the host's Node path.** `process.execPath` does not exist inside the image, so every sandboxed probe would exit 127 and map baseline, control, every mutant, every hunk and the revert to inconclusive — zero evidence exactly when containment is on. It uses the image's `node` now; the vitest path resolves because it lives under the mount. - **The mount root took the first `.qwen/tmp`, not the deepest.** A review run from inside another review's worktree nests them, and the first occurrence widens the mount to the outer temp dir — pulling `<repo>/.git` and every sibling checkout in, which is the one property the mount exists for. - **No UID/GID mapping.** The default image runs as root, so the container's writes into the mounted trees were root-owned and every later host-side cleanup — the install-timeout `rmSync`, `discardWorktree`, the sweeps — hit EACCES, accumulating residue across reviews. Two things the fixes themselves needed, found by checking them rather than by being told: - **Refusing with `toolchain: 'unsupported'` would have caused the regression it was closing.** That value has a documented meaning — the brief reads it as "build-test could not scope this repo, install and build it yourself" — so a refusal routed into it would have sent the agent to run the reviewed code by hand, unsandboxed. It is a distinct `refused` now, with a brief rule that says the evidence is unavailable and must NOT be reconstructed by hand. - **A bare `--user uid:gid` resets `$HOME` to `/`**, which the mapped user cannot write, so npm fails before the install starts — `utils/sandbox.ts` copies the host's `$HOME` for exactly this. The container gets a writable HOME inside the mount, with the npm cache under it. The duplicated mount-root arithmetic became one exported `mountRootFor`, which is both how the copies stopped drifting and how the nested case got a test. Every fix above is pinned by a test that goes red when that fix alone is reverted.
|
All five Criticals handled in
The host Node path — confirmed, and the consequence is worse than "some probes fail": exit 127 with empty stdout maps baseline, control, every mutant, every hunk and the revert to inconclusive, so the phase yields nothing exactly when containment is on. Fixed as you suggested; the vitest path resolves because it lives under the mount.
UID/GID — confirmed, and the residue argument is the one that decided it: root-owned writes make every later host-side cleanup EACCES, which is the cross-run-state class #9221 spent rounds closing. Two things the fixes themselves needed, which I found by checking them rather than by being told, and which are worth recording because both would have shipped as new defects:
The gitfile finding I have not fixed, and I want to be exact about why rather than let silence read as agreement. You are right on the mechanism: each tree's What I am not doing in this PR is the post-run verification, for a reason I would rather state than assume: this is not a hole the PR opens. Unsandboxed — today's behaviour, and the default this PR keeps — the reviewed code runs as the invoking user and can rewrite any of those gitfiles already, with no container involved. The containment narrows the blast radius and does not create this route. Fixing it properly means requiring every tree's gitdir to resolve under Local: 中文说明五个 Critical 已在
宿主 Node 路径 —— 确认，且后果比"部分探针失败"更糟：exit 127 + 空 stdout 会把基线、对照、每个突变体、每个 hunk 与回退全部映射为 inconclusive——恰恰在容器化开启时该阶段产出为零。已按你的建议修正；vitest 路径能解析，因为它位于挂载之内。
UID/GID —— 确认，而决定性的是残留那条论证：root 所有的写入会让其后每一次宿主侧清理 EACCES，那正是 #9221 花了若干轮才关掉的跨运行状态类别。 两件修复本身需要的东西，是我核查修复时发现的、而非被指出的，值得记录，因为两者都会作为新缺陷出厂：
gitfile 那条我没有修，我宁愿把理由说准，也不愿让沉默读作认同。 机制上你是对的：每棵树的 本 PR 不做的是运行后校验，理由我宁可明说而非默认：这不是本 PR 打开的洞。在无沙箱状态下——也就是今天的行为、以及本 PR 保持的默认——被审代码以调用者身份运行，本就能重写那些 gitfile 中的任意一个，与容器无关。容器化缩小了影响面，并没有创造这条路径。要修对，需要在每个宿主侧消费点（按你的清单：探针树创建与恢复）要求每棵树的 gitdir 解析到 本地： |
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 — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform-fragile assertions bite exactly there.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out assertion arm is environment-dependent.
Not explored to full depth (tool budget reached): "agent test-matrix": none — no check was cut short..
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.ts:165 — [review] R1-6 the disclose strings are produced but never surfacedpackages/cli/src/commands/review/lib/sandboxed-exec.ts:99 — [review] R1-8 an unrecognized QWEN_REVIEW_SANDBOX value silently falls through to offpackages/cli/src/commands/review/lib/sandboxed-exec.ts:101 — [review] R1-9 policy parsing asymmetric: env normalized, settings exact-matchedpackages/cli/src/commands/review/lib/review-settings.ts:17 — [review] R1-10 a settings-load failure silently degrades required to offpackages/cli/src/commands/review/lib/sandboxed-exec.ts:201 — [review] R1-11 the env allowlist drops the proxy variablespackages/cli/src/commands/review/lib/sandboxed-exec.ts:228 — [review] R1-12 the container starts with no pids/memory/CPU limitspackages/cli/src/commands/review/build-test.ts:346 — [review] R1-13 the single RW mount gives every containerized command write access to all sibling treespackages/cli/src/commands/review/test-efficacy.ts:1730 — [review] R1-14 the composed sh -lc suite command is untestedpackages/cli/src/commands/review/lib/review-settings.ts:110 — [review] R1-15 review.sandbox has no test at either the loader or the schema layerpackages/cli/src/commands/review/test-efficacy.ts:1526 — [review] R1-16 orphaned doc comment (probeContainer inserted between doc and function)packages/cli/src/commands/review/build-test.ts:321 — [review] R1-17 orphaned doc comment (containerised inserted after run()'s doc)
[Critical] R2-4 (packages/cli/src/commands/review/test-delta.ts:223,:315 — file not in this diff, so this blocker cannot be anchored inline): test-delta never crosses the containment choke point. The diff makes build-test's run() the containment choke point and gates two phases at phase-top, but runTestDelta keeps its own private spawnSync(command, { shell: true, env: buildRunEnv(process.env) }) with no refusal gate and no containerised() consultation — newly wrong because of this diff, whose own agent-briefs bullet routes the agent to base-tree + test-delta whenever tests failed. Both arms reachable: with a runtime answering, a containerized build-test records failing tests and the documented flow reruns them BASE-SIDE UNCONTAINED; with no runtime, a stale report from an earlier run/regime does the same. Second consequence under auto: PR-side suites run in the restricted container env while base-side runs in the full host env, so an env-sensitive test can flip on exactly one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. witness: [probe] QWEN_REVIEW_SANDBOX=required with answering docker, runTestDelta driven through the PR's own code: verdict {kind:'container',runtime:'docker'}; DELTA base-side output 'SECRET-IS:[hunter2-credential]' — the recorded repo command ran directly in the host shell with the full environment. Fix direction: route test-delta's rerun through build-test's run() (or export and apply containerised there); a bare refuseUnsandboxedPhase call does not close the answering-runtime arm.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform-fragile assertions bite exactly there。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out assertion arm is environment-dependent。
未探索到全部深度（达到工具调用预算）："agent test-matrix"：none — no check was cut short.。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 11 条（原文未翻译，列表见上方英文部分）。
[Critical] R2-4 (packages/cli/src/commands/review/test-delta.ts:223,:315 — file not in this diff, so this blocker cannot be anchored inline): test-delta never crosses the containment choke point. The diff makes build-test's run() the containment choke point and gates two phases at phase-top, but runTestDelta keeps its own private spawnSync(command, { shell: true, env: buildRunEnv(process.env) }) with no refusal gate and no containerised() consultation — newly wrong because of this diff, whose own agent-briefs bullet routes the agent to base-tree + test-delta whenever tests failed. Both arms reachable: with a runtime answering, a containerized build-test records failing tests and the documented flow reruns them BASE-SIDE UNCONTAINED; with no runtime, a stale report from an earlier run/regime does the same. Second consequence under auto: PR-side suites run in the restricted container env while base-side runs in the full host env, so an env-sensitive test can flip on exactly one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. witness: [probe] QWEN_REVIEW_SANDBOX=required with answering docker, runTestDelta driven through the PR's own code: verdict {kind:'container',runtime:'docker'}; DELTA base-side output 'SECRET-IS:[hunter2-credential]' — the recorded repo command ran directly in the host shell with the full environment. Fix direction: route test-delta's rerun through build-test's run() (or export and apply containerised there); a bare refuseUnsandboxedPhase call does not close the answering-runtime arm.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| export function refuseUnsandboxedPhase( | ||
| verdict: SandboxVerdict = sandboxVerdict(), | ||
| ): string | null { |
There was a problem hiding this comment.
[Critical] R1-1: round 1's blocker still stands — the fix covers only the no-runtime arm. refuseUnsandboxedPhase() models "did a runtime answer", never "can this phase actually be contained": with a runtime ANSWERING, the verdict is container, the gate returns null, and every route the container cannot serve still runs the reviewed code unsandboxed with the full environment.
Measured at this head: a local-checkout review (cwd outside .qwen/tmp) falls through containerised() → run() and spawns with buildRunEnv() — the entire process.env — with the report indistinguishable from a contained run; test-efficacy's probe tree (<worktree>-probe) is unmountable for a local checkout, so the direct spawn inherits the whole environment; and the unsupported-toolchain hand-off passes the gate on verdict container, instructing the agent to run install/build/test in its own shell, policy never consulted. The settings schema promises operators that required refuses to run them unsandboxed, and #9556's policy table says the execution-dependent steps report UNAVAILABLE — not "run directly".
witness: probe through the real run() with answering docker, policy=required
ARM-A (cwd outside .qwen/tmp): exit 0, output "SECRET-IS:[hunter2-credential]" — ran directly, saw the host secret
ARM-B (cwd inside .qwen/tmp): exit 0, output "SECRET-IS:[]" — contained, allowlist stripped the secret
Make the phase-top decision containment-aware: under required, refuse when the phase tree cannot be mounted (e.g. thread the phase root through and return a reason when mountRootFor(root) === null), and treat the unsupported-toolchain hand-off as a refusal under required — the phase reports evidence-unavailable instead of spawning or instructing.
中文说明
第 1 轮的阻断项 R1-1 依旧成立——本次修复只覆盖了"无运行时"这一臂。refuseUnsandboxedPhase() 只建模"有没有运行时应答"，从不建模"本阶段是否真的能被容器化"：当有运行时应答时，判定为 container，关卡放行，凡是容器无法承接的路径依旧带着完整环境在沙箱外运行被审代码。
在本 head 上实测：本地检出审查（cwd 不在 .qwen/tmp 之内）经 containerised() 落空后从 run() 直接 spawn，带 buildRunEnv()——整个 process.env——且报告与容器化运行无法区分；test-efficacy 的探针树（<worktree>-probe）在本地检出下不可挂载，直接 spawn 继承全部环境；不支持工具链的交接路径在 container 判定下过关，指示 agent 在自己的 shell 里运行 install/build/test，全程不查询策略。设置 schema 向操作者承诺 required 拒绝在沙箱外运行，#9556 的策略表也写明依赖执行的步骤"报告不可用"——而不是"直接运行"。
证据：探针驱动真实 run()（有应答的 docker，policy=required）——ARM-A（cwd 在 .qwen/tmp 外）：exit 0，输出 SECRET-IS:[hunter2-credential]，直接运行、看到了宿主密钥；ARM-B（cwd 在 .qwen/tmp 内）：exit 0，输出 SECRET-IS:[]，已容器化、白名单剥掉了密钥。
请让阶段顶部的决定具备"可容器化"意识：在 required 下，当阶段树无法挂载时拒绝（例如把阶段根传入，mountRootFor(root) === null 时返回原因），并把不支持工具链的交接也视为 required 下的拒绝——阶段报告"证据不可用"，而不是 spawn 或指示。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // file one run rewrote, an ignored plant a sweep honoured. `--rm` is | ||
| // isolation by construction rather than by hygiene. | ||
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands at this head — deliberately deferred by the author, mechanism unchanged. The RW mount covers every pipeline tree's .git gitfile: it sits inside its tree, i.e. inside this mount, and code in the container can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge. The two host-side consumers that follow the redirect — probe-tree creation (git worktree add with the review worktree as cwd) and restoreProbeTreeTracked after every sandboxed suite run — have neither scratch-tree.ts's gitfile verification nor its localFilterCommands refusal; the restore's backpointer round-trip passes because the attacker writes the very gitdir file the check reads, and its -c overrides neutralize core.hooksPath/core.fsmonitor but NOT filter.*.
witness: round-1 end-to-end reproduction (git 2.43.0, throwaway trees mirroring the layout):
check sequence passed (backpointer round-trip read the attacker-planted gitdir), checkout exit 0,
HOST-SIDE EXECUTION OBSERVED for both the restore checkout and `git worktree add`;
scratch-tree's identity gate refused the same tree.
The module doc's "<repo>/.git stays outside it" is true of the common dir and false of this security property. Treat the per-tree gitfile as attack surface: after each sandboxed run verify every tree's .git still resolves to its own admin entry under <repo>/.git/worktrees/ (outside the mount) and refuse on mismatch; extend scratch-tree's localFilterCommands refusal and gitfile check to probe-tree creation and restoreProbeTreeTracked — or land the documented follow-up before auto/required are turned on in CI. The doc correction committed to in round 1 is also still absent (see the separate comment at the module doc).
中文说明
R1-3 在本 head 上依旧成立——作者有意推迟到后续 PR，机制未变。RW 挂载覆盖了每棵流水线树的 .git gitfile：它位于树内、即在此挂载之内，容器内代码可以把它改写为 gitdir: <同一挂载下植入的目录>，其中带 filter.<x>.smudge。两个会跟随重定向的宿主侧消费者——探针树创建（以 review 工作树为 cwd 的 git worktree add）与每次沙箱化套件运行后的 restoreProbeTreeTracked——既没有 scratch-tree.ts 的 gitfile 校验、也没有其 localFilterCommands 拒绝；restore 的回指针往返检查会通过，因为攻击者写下的正是该检查读取的 gitdir 文件，其 -c 覆盖能中和 core.hooksPath/core.fsmonitor，却中和不了 filter.*。
证据：第 1 轮已在 git 2.43.0 上按该布局端到端复现：检查序列通过（回指针往返读到攻击者植入的 gitdir）、checkout exit 0、restore checkout 与 git worktree add 均观测到宿主侧执行；scratch-tree 的身份关卡对同一棵树会拒绝。
模块文档的"<repo>/.git 留在挂载外"对 common dir 为真，对这条安全性质为假。请把每棵树的 gitfile 当作攻击面：每次沙箱化运行后校验各树 .git 仍解析到 <repo>/.git/worktrees/（挂载外）下自己的 admin entry，不一致即拒绝；把 scratch-tree 的 localFilterCommands 拒绝与 gitfile 检查扩展到探针树创建与 restoreProbeTreeTracked——或者在 CI 打开 auto/required 之前先落地已承诺的后续修复。第 1 轮承诺的文档更正也仍未做（见模块文档处的另一条评论）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const refusal = refuseUnsandboxedPhase(); | ||
| if (refusal) { |
There was a problem hiding this comment.
[Critical] A policy refusal on a --resume call RETURNS a fresh report instead of throwing, so the handler's unconditional writeFileSync(args.out, …) overwrites the in-flight report the call was asked to continue — destroying it and killing the resume chain permanently. The refusal report carries no run identity field, so every later --resume — even after the runtime recovers — fails previousReport's identity check ("records no run identity"), and the round must redo install + build + all suites from scratch. Trigger: policy required, call 1 runs containerized and writes a partial report when the whole-call budget expires mid-suites (the ordinary --resume case); before the resume call the runtime probe fails (daemon restarted, or the 30s probe times out under load). The !adapter branch twenty lines below codifies the exact invariant this violates: "A continuation must never answer with a FRESH report … Throwing reaches the handler's catch, which writes nothing."
witness: three-call sequence through the real runBuildTest (runtime stubbed to fail on call 2):
CALL1 toolchain npm with run identity
CALL2 returned toolchain=refused run=null; OUT after call2: toolchain=refused run=null test entries=0 — original overwritten
CALL3 threw 'records no run identity'
FLIP (throw when previous set): CALL2 threw, OUT preserved (run present=true), CALL3 resumed
| const refusal = refuseUnsandboxedPhase(); | |
| if (refusal) { | |
| const refusal = refuseUnsandboxedPhase(); | |
| if (refusal) { | |
| if (previous) { | |
| throw new Error( | |
| `build-test: --resume cannot continue the run recorded at ${args.out}: ` + | |
| `${refusal} — running the remaining suites unsandboxed is what the policy ` + | |
| `forbids. The report is left untouched; get a container runtime answering, ` + | |
| `then resume again.`, | |
| ); | |
| } |
中文说明
策略拒绝在 --resume 调用上返回一份全新报告而不是抛错，于是处理器无条件的 writeFileSync(args.out, …) 会覆盖它本应续跑的那份在途报告——证据被毁、续跑链被永久杀死。拒绝报告不带 run 身份字段，因此之后每一次 --resume——即使运行时已恢复——都会撞上 previousReport 的身份检查（"records no run identity"），整轮只能从头重做 install + build + 全部套件。触发路径：策略 required，第 1 次调用容器化运行、在整套预算于套件中途耗尽时写下部分报告（正是 --resume 存在的常规场景）；续跑调用前运行时探测失败（守护进程重启，或 30s 探测在负载下超时）。下方二十行的 !adapter 分支恰好写明了被此处违反的不变量："续跑绝不能以一份全新报告作答……抛错会到达处理器的 catch，那里什么都不写。"
证据：对真实 runBuildTest 的三调用序列（第 2 次调用前把运行时置为失败）——第 1 次调用得到 npm 工具链与运行身份；第 2 次调用返回 toolchain=refused、run=null，调用后报告文件变为 toolchain=refused、run=null、test 条目 0（原报告被覆盖）；第 3 次调用抛 "records no run identity"。翻转为"有 previous 时抛错"后：第 2 次调用抛错、报告保留（run 仍在）、第 3 次调用成功续跑。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| cwd: join(tmpDir, 'review-pr-9'), | ||
| kind: 'install', | ||
| }); | ||
| const user = args[args.indexOf('--user') + 1]; |
There was a problem hiding this comment.
[Critical] Pattern (1 of 3 locations): three assertions in this new file only hold on POSIX with a host-typical environment — the implementations are correct, the tests are platform-fragile, and the Windows merge-queue gate runs them. THIS assertion: containerCommand emits --user only when process.getuid/getgid exist and the documented SANDBOX_SET_UID_GID opt-out is unset, but the test asserts unconditionally. On Windows process.getuid is undefined, args.indexOf('--user') is -1, and args[0] ('run') is compared against 'undefined:undefined'; on POSIX any shell carrying the documented opt-out (docs/users/features/sandbox.md names export SANDBOX_SET_UID_GID=false) fails it spuriously. ci.yml:875 test_windows (merge_group event) runs npm run test:ci → vitest run in packages/cli, so this file red-boards the required Windows check.
witness: SANDBOX_SET_UID_GID=false npx vitest run sandboxed-exec.test.ts
× containerCommand > maps the host uid … AssertionError: expected 'run' to be '1000:1000' (1 failed | 12 passed)
flip (env unset): 13 passed
Make it hermetic and platform-aware, the utils/sandbox.test.ts convention: it.skipIf(!process.getuid || process.env['SANDBOX_SET_UID_GID']?.toLowerCase().trim() === 'false') plus vi.stubEnv('SANDBOX_SET_UID_GID', 'true') inside.
中文说明
模式（3 处之 1）：这个新文件里有三处断言只在"POSIX + 宿主典型环境"下成立——实现是对的，测试是平台脆弱的，而 Windows 合并队列关卡会运行它们。本处断言：containerCommand 只在 process.getuid/getgid 存在且未设置文档化的 SANDBOX_SET_UID_GID 豁免时才发出 --user，但测试无条件断言。Windows 上 process.getuid 为 undefined，args.indexOf('--user') 为 -1，args[0]（'run'）会与 'undefined:undefined' 比较；POSIX 上任何带文档化豁免变量（docs/users/features/sandbox.md 写明 export SANDBOX_SET_UID_GID=false）的 shell 都会让它假失败。ci.yml:875 的 test_windows（merge_group 事件）运行 npm run test:ci → packages/cli 的 vitest run，本文件会让必需的 Windows 检查变红。
证据：SANDBOX_SET_UID_GID=false 下运行该测试 → × maps the host uid … expected 'run' to be '1000:1000'（1 failed | 12 passed）；去掉该变量 → 13 passed。
请改成密封且平台感知，沿用 utils/sandbox.test.ts 的约定：it.skipIf(!process.getuid || process.env['SANDBOX_SET_UID_GID']?.toLowerCase().trim() === 'false')，并在测试内 vi.stubEnv('SANDBOX_SET_UID_GID', 'true')。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // HOME inside the mount: forcing a uid resets it to `/`, which the | ||
| // mapped user cannot write, and npm fails before the install starts. | ||
| 'HOME=/home-in-mount', | ||
| 'npm_config_cache=/home-in-mount/.npm', |
There was a problem hiding this comment.
[Critical] Pattern (2 of 3 locations): platform-fragile assertions in this new file (see the comment at line 168 for the pattern and the Windows merge-queue gate). THIS assertion hardcodes the POSIX literal 'npm_config_cache=/home-in-mount/.npm', but containerEnv builds the entry with path.join(homeDir, '.npm') — on Windows node:path IS path.win32, so the implementation emits backslashes and the toEqual fails (the HOME=/home-in-mount entry above it passes — template literal, no join).
witness: node v22.23.0 path.win32 probe:
join('/home-in-mount', '.npm') = '\home-in-mount\.npm' vs expected '/home-in-mount/.npm'
Build the expected entry with the same primitive the implementation uses: \npm_config_cache=${join('/home-in-mount', '.npm')}``.
中文说明
模式（3 处之 2）：本新文件中的平台脆弱断言（模式与 Windows 合并队列关卡见第 168 行的评论）。本处断言硬编码了 POSIX 字面量 'npm_config_cache=/home-in-mount/.npm'，但 containerEnv 用 path.join(homeDir, '.npm') 构造该条目——Windows 上 node:path 就是 path.win32，实现会输出反斜杠，toEqual 因而失败（上面那条 HOME=/home-in-mount 能过，因为它是模板字符串、未经 join）。
证据：node v22.23.0 的 path.win32 探针：join('/home-in-mount', '.npm') = '\home-in-mount\.npm'，与期望值 '/home-in-mount/.npm' 不符。
请用与实现相同的原语构造期望值：\npm_config_cache=${join('/home-in-mount', '.npm')}``。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| cwd: join(tmpDir, 'review-pr-9'), | ||
| kind: 'install', | ||
| }); | ||
| expect(args.slice(-3)).toEqual(['sh', '-lc', 'npm ci && npm test']); |
There was a problem hiding this comment.
[Suggestion] Pattern (5 of 5 locations): under-pinned wiring in this file (see the comment at line 76). HERE: no assertion pins that containerCommand puts the IMAGE in the argv — the fixture image: 'example/image:tag' is only fed in, never checked, and this file is the only suite exercising containerCommand. Deleting opts.image from the final push keeps every assertion passing (slice(-3) still sees ['sh', '-lc', cmd]), while at runtime docker parses the first positional (sh) as the image and every containerized install/build/test fails with "Unable to find image 'sh:latest'" — surfacing only in a live containerized run, which this suite never performs.
witness: mutant 'drop opts.image' → suite 13/13 green; flip pin expect(args.slice(-4)).toEqual([image, 'sh', '-lc', cmd]) → red against mutant (1 failed | 12 passed), green against pristine
Extend the tail assertion: expect(args.slice(-4)).toEqual([base.image, 'sh', '-lc', 'npm ci && npm test']);.
中文说明
模式（5 处之 5）：本文件对接线钉得不够（见第 76 行的评论）。本处：没有断言钉住 containerCommand 把镜像放进 argv——夹具 image: 'example/image:tag' 只被传入、从未被检查，而本文件是唯一运行 containerCommand 的套件。从最终 push 中删掉 opts.image，所有断言依旧通过（slice(-3) 仍看到 ['sh', '-lc', cmd]），而运行时 docker 会把第一个位置参数（sh）当作镜像，所有容器化 install/构建/测试都会以 "Unable to find image 'sh:latest'" 失败——只在真实容器化运行中暴露，而本套件从不做这种运行。
证据：突变体"删掉 opts.image" → 套件 13/13 绿；翻转钉 expect(args.slice(-4)).toEqual([image, 'sh', '-lc', cmd]) → 对突变体红（1 failed | 12 passed）、对原代码绿。
请扩展尾部断言：expect(args.slice(-4)).toEqual([base.image, 'sh', '-lc', 'npm ci && npm test']);。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // bought by running that suite unsandboxed. Refusing here rather than at the | ||
| // spawn keeps the report's vocabulary intact: the phase produced nothing, and | ||
| // says why, instead of a run of probes each blaming the runner. | ||
| const sandboxRefusal = refuseUnsandboxedPhase(); |
There was a problem hiding this comment.
[Suggestion] Pattern (1 of 4 locations): the diff's production wiring has no coverage in the consumer suites. HERE: the two phase-level refusal gates — the entire round-1 fix ("make required actually refuse") — have zero test coverage at either call site; the only test of refuseUnsandboxedPhase calls it directly, never through a phase. Deleting this early-return (or the twin in build-test.ts:812, or moving either after adapter selection / probe-tree creation) leaves every test green, and under required with no runtime the phases again execute the reviewed repo's commands unsandboxed — the exact regression this commit exists to prevent. Both gates are deterministically testable by mocking ./sandboxed-exec.js.
witness: grep across packages/cli test tree — zero refusal/sandbox references in build-test.test.ts and test-efficacy.test.ts; only sandboxed-exec.test.ts touches the gate, calling it directly
Add one test per phase: mock sandboxed-exec.js so refuseUnsandboxedPhase returns a reason; assert runBuildTest returns toolchain: 'refused', ok: false, the note, and never calls the injected exec; and runTestEfficacy records the note and runs zero probes.
中文说明
模式（4 处之 1）：本 diff 的生产接线在消费者套件中没有覆盖。本处：两个阶段级拒绝关卡——第 1 轮修复的全部（"让 required 真的拒绝"）——在两个调用点都零测试覆盖；refuseUnsandboxedPhase 唯一的测试是直接调用它，从不经过阶段。删除这个提前返回（或 build-test.ts:812 的孪生关卡，或把任一者移到适配器选择/探针树创建之后）所有测试保持绿，而在无运行时的 required 下，两个阶段会再次在沙箱外执行被审仓库的命令——正是本提交要防止的回归。两个关卡都可以通过 mock ./sandboxed-exec.js 做确定性测试。
证据：对 packages/cli 测试树全量 grep——build-test.test.ts 与 test-efficacy.test.ts 中零拒绝/沙箱引用；只有 sandboxed-exec.test.ts 触及该关卡，且是直接调用。
请为每个阶段各加一个测试：mock sandboxed-exec.js 使 refuseUnsandboxedPhase 返回原因；断言 runBuildTest 返回 toolchain: 'refused'、ok: false、带 note，且从不调用注入的 exec；断言 runTestEfficacy 记录 note 且零探针运行。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // install scripts the PR committed, its build, its suite — so it is the | ||
| // thing #9556 is about. `containerised` returns null when the run is not | ||
| // sandboxed, and the direct spawn below is unchanged for that case. | ||
| const boxed = containerised(command, cwd, kind); |
There was a problem hiding this comment.
[Suggestion] Pattern (2 of 4 locations): the diff's production wiring has no coverage in the consumer suites (see the comment at test-efficacy.ts:2488). HERE: the wiring between the tested containerCommand argv builder and the actual spawn — containerised()'s three null cases, the boxed/direct ternary, and the boxed branch's env: process.env without shell: true — is exercised by no test; every build-test.test.ts case runs policy off, so containerised() only ever takes its first null path in tests. Removing if (tmpDir === null) return null; stays green: a local-checkout review would then be handed container argv with a mount that doesn't contain the tree; swapping the ternary or restoring shell: true/buildRunEnv() on the boxed branch regresses silently.
witness: mutant 'remove tmpDir===null return' → build-test + sandboxed-exec suites green (118/118);
probe (policy auto, fake docker answering): mutated tree → docker run invoked with null mount (red); correct tree → no docker run (green)
Mock sandboxed-exec.js to a container verdict and assert run() spawns (boxed.file, boxed.args) without a shell for a cwd under a temp dir, and falls back to direct spawn for a cwd outside one.
中文说明
模式（4 处之 2）：本 diff 的生产接线在消费者套件中没有覆盖（见 test-efficacy.ts:2488 的评论）。本处：已测试的 containerCommand argv 构造器与实际 spawn 之间的接线——containerised() 的三种 null 情形、容器/直接三元选择、容器分支不带 shell: true 的 env: process.env——没有任何测试运行；build-test.test.ts 的所有用例都以策略 off 运行，测试中 containerised() 只走第一个 null 分支。移除 if (tmpDir === null) return null; 仍是绿：本地检出审查随后会拿到挂载不含该树的容器 argv；交换三元、或在容器分支恢复 shell: true/buildRunEnv() 都会静默回归。
证据：突变体"移除 tmpDir===null 返回" → build-test + sandboxed-exec 套件绿（118/118）；探针（策略 auto、假 docker 应答）：突变树 → 以 null 挂载调用 docker run（红）；正确树 → 不调用 docker run（绿）。
请 mock sandboxed-exec.js 为 container 判定，断言对临时目录内的 cwd，run() 以 (boxed.file, boxed.args) 无 shell spawn；对临时目录外的 cwd 回退直接 spawn。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| Math.min(perCommandMs, remainingMs()), | ||
| // The one command that needs the registry. Everything else this adapter | ||
| // runs is offline under the sandbox policy — see `containerCommand`. | ||
| 'install', |
There was a problem hiding this comment.
[Suggestion] Pattern (3 of 4 locations): the diff's production wiring has no coverage in the consumer suites (see the comment at test-efficacy.ts:2488). HERE: the only call site that grants network under the sandbox policy is unpinned — npm-toolchain.test.ts's okExec accepts (command: string) only, so no test observes the fourth kind argument. Deleting 'install' stays green: run()'s default kind = 'test' then applies --network none to npm ci, and every sandboxed build-test fails at install — read by the pipeline as an install failure of the reviewed repo rather than a policy wiring bug. The inverse mutation (passing 'install' for build/test) likewise stays green and would grant egress to builds the design keeps offline.
witness: mutant 'delete install arg' → npm-toolchain 7/7, sandboxed-exec 13/13, build-test 105/105 all green;
inverse mutant → green; probe capturing the 4th arg: green on correct code (install → 'install'), red on both mutants
Capture okExec's fourth argument and assert the install command receives 'install' while build/test commands receive 'test' or omit it.
中文说明
模式（4 处之 3）：本 diff 的生产接线在消费者套件中没有覆盖（见 test-efficacy.ts:2488 的评论）。本处：沙箱策略下唯一授予网络的调用点没有被钉住——npm-toolchain.test.ts 的 okExec 只接受 (command: string)，没有测试观察第四个 kind 参数。删掉 'install' 仍是绿：run() 的默认 kind = 'test' 会对 npm ci 施加 --network none，所有沙箱化 build-test 都会在安装时失败——流水线会把这读成被审仓库的安装失败，而不是策略接线 bug。反向突变（给 build/test 传 'install'）同样是绿，会把出口网络授予设计上保持离线的构建。
证据：突变体"删除 install 参数" → npm-toolchain 7/7、sandboxed-exec 13/13、build-test 105/105 全绿；反向突变体 → 绿；捕获第 4 参数的探针：对正确代码绿（install → 'install'），对两个突变体都红。
请捕获 okExec 的第四个参数，断言 install 命令收到 'install'，而 build/test 命令收到 'test' 或省略。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // maps every probe — baseline, control, each mutant, each hunk, the revert — | ||
| // to inconclusive, blaming the runner's output for a wiring error. The vitest | ||
| // bin path DOES resolve, because it lives under the mounted temp dir. | ||
| const suite = `node ${shellQuotePath( |
There was a problem hiding this comment.
[Suggestion] Pattern (4 of 4 locations): the diff's production wiring has no coverage in the consumer suites (see the comment at test-efficacy.ts:2488). HERE: the sandboxed probe-run path is untested end to end — the probeContainer boxed/direct choice and the image-PATH node decision have no test anywhere (the suite-quoting half of this gap is round-1 ledger entry R1-14, still open). The boxed route is the first place probe paths reach a shell: a regression in probeContainer's null cases or in the node-off-image-PATH choice maps every probe — baseline, control, each mutant, hunk, revert — to inconclusive, silently misattributing test efficacy.
witness: grep across all tests — no coverage of probeContainer, the boxed/direct choice, or the suite string;
test-efficacy.integration.test.ts drives runProbeSuite with an empty probe list ('is not the subject')
Assert via an injected seam (or an exported helper) that the boxed path uses image-PATH node and probeContainer returns null for the three documented cases.
中文说明
模式（4 处之 4）：本 diff 的生产接线在消费者套件中没有覆盖（见 test-efficacy.ts:2488 的评论）。本处：沙箱化探针运行路径端到端无测试——probeContainer 的容器/直接选择与"镜像 PATH 上的 node"决定在任何地方都没有测试（此缺口的套件引号部分即第 1 轮账目 R1-14，仍未关闭）。容器分支是探针路径第一次进入 shell 的地方：probeContainer 的 null 情形或"镜像 PATH 的 node"选择一旦回归，会把每个探针——基线、对照、每个突变体、hunk、回退——判为 inconclusive，静默错判测试效力。
证据：对全部测试 grep——probeContainer、容器/直接选择、套件字符串均无覆盖；test-efficacy.integration.test.ts 以空探针列表驱动 runProbeSuite（"不是其主题"）。
请通过注入缝隙（或导出的辅助函数）断言容器分支使用镜像 PATH 的 node，且 probeContainer 对三种文档化情形返回 null。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…inment (#9556) Round 2. The sharpest finding is that the guarantee this PR advertises was true of one route and false through another. **A repository could switch off the containment that exists to contain it.** `operatorReviewSettings` skips the workspace scope precisely so a `.qwen/settings.json` cannot set review policy — but the env layer that outranked it is repository-controlled too: `loadEnvironment` walks up from cwd and applies `<repo>/.qwen/.env`, from the very checkout under review, admitted by default because folder trust starts off. `QWEN_REVIEW_SANDBOX=off` in a committed `.env` disabled it. Three siblings were worse, because they have no ordering to fall back on: `QWEN_REVIEW_SANDBOX_IMAGE` chooses the image the reviewed code runs *inside*; `SANDBOX_SET_UID_GID=false` puts the container back to root; `DOCKER_HOST` chooses which daemon answers, so `required` reads as satisfied and whatever that daemon returns is scored as evidence. `environment.ts` gains `isFileSourcedEnvKey`, and containment now reads only the operator's settings or a real process variable. The policy additionally only ever tightens, so even a genuine env value cannot lower a settings `required` — which, as a mutation showed, is what actually protects the policy; the file-source check is what protects the other three. **`required` still failed open where the mount could not be built.** The gate asked "did a runtime answer", never "can this phase be contained": with a healthy daemon and a cwd outside `.qwen/tmp` — a `/review` of a local checkout — the command fell through to the direct spawn with the full environment and a report indistinguishable from a contained run. It asks the second question now. **A refusal on `--resume` destroyed the run it was asked to continue.** Returning a report let the handler's unconditional write overwrite the in-flight one, and the refusal carries no run identity, so every later resume failed the identity check even after the runtime recovered. It throws on a continuation, which is the invariant the `!adapter` branch states in its own words. **The HOME added last round was itself cross-run state.** It lived on the shared mount, and `sh -lc` sources `$HOME/.profile` while npm reads `$HOME/.npmrc` — so one run's postinstall could plant what the next review's install executes, with the network on. That contradicted this module's own `--rm` "isolation by construction" claim, and it arrived with the fix for the `$HOME` problem rather than in the original. HOME is a tmpfs now: discarded with the container, never on the host. The npm cache stays on the mount, and the comment says plainly that npm's integrity check is what stands between a poisoned cache and a bad install. **The mount root was lexical.** `resolve` never touches the filesystem, so a symlink at or above `.qwen/tmp` — committable as mode 120000 — would have widened a read-write bind mount to wherever it pointed. Every other creating or destroying path in this pipeline refuses that; this one does now too. Also: the new tests were platform-fragile in three places (Windows has no `process.getuid`, and the documented `SANDBOX_SET_UID_GID` opt-out could fail them on a developer's box), and the secret-leak check asserted against whatever the runner happened to export rather than a planted canary. Both fixed, and the mount-root tests now build real directories, which is how the symlink refusal got pinned at all. Every fix here is pinned by a test that goes red when that fix alone is reverted — except the policy's file-source check, which a mutation showed is redundant with the tightening rule, and which is documented as defence in depth rather than claimed as load-bearing.
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 — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file and the Windows-only case-folding path (R3-21) bite exactly there.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.ts:174 — [review] R2-8 JSDoc claims a SANDBOX-set session 'returns direct' — contradicts the NOTE and the codepackages/cli/src/commands/review/lib/sandboxed-exec.ts:40 — [review] R2-9 the R1-3 doc correction (per-tree gitfiles sit INSIDE the mount) is still absentpackages/cli/src/commands/review/lib/sandboxed-exec.ts:420 — [review] R2-10 opt-out honours only 'false', not '0', despite the 'same opt-out' commentpackages/cli/src/commands/review/test-efficacy.ts:2497 — [review] R2-11 refusal note fires even when the phase had nothing to probepackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:169 — [review] R2-12 (residual) auto-with-no-runtime fall-through is still unpinnedpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:40 — [review] R2-13 env-vs-setting merge direction unpinned (expectation now 'required' under tighten-only)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts — [review] R2-14 --workdir unpinned for the install-kind invocationspackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:333 — [review] R2-16 image slot in argv unpinned (incl. the DEFAULT_IMAGE-substitution mutant)packages/cli/src/commands/review/test-efficacy.ts:2496 — [review] R2-17 phase-level refusal gates untested in the consumer suites (now incl. the resume-throw arm)packages/cli/src/commands/review/build-test.ts:376 — [review] R2-18 containerised/run wiring untested in the consumer suitespackages/cli/src/commands/review/lib/npm-toolchain.ts:803 — [review] R2-19 the 'install' kind argument is unobserved by any test (okExec signature)packages/cli/src/commands/review/test-efficacy.ts:1738 — [review] R2-20 the sandboxed probe-run path is untested end to endpackages/cli/src/commands/review/build-test.ts:833 — [review] D3-1 the refused report is cemented as a settled base-build failure by base-tree's marker (anchored on unchanged code — age rule)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:202 — [probe] D3-2 podman runtime passthrough untested — hardcoded-docker mutant ships green (anchored on unchanged code — age rule)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:323 — [probe] D3-3 --init unpinned — deleting it ships green and breaks the timeout kill (anchored on unchanged code — age rule)
Convergence: round 3 posted 16 inline comment(s), 14 of them reported for the first time; the previous round posted 22 (20 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/sandboxed-exec.ts (findings in rounds 1, 2; 6 more now); packages/cli/src/commands/review/lib/sandboxed-exec.test.ts (findings in round 2; 5 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.)
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179,:315 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment choke point. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on exactly one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. witness: round-2 probe — QWEN_REVIEW_SANDBOX=required with answering docker, runTestDelta driven through the PR's own code: DELTA base-side output 'SECRET-IS:[hunter2-credential]' (the recorded repo command ran directly in the host shell with the full environment); re-verified at 04a6ee1: the private spawnSync is unchanged and the file still has no gate. Fix direction: route test-delta's rerun through build-test's run() (or add the same phase-top gate over the baseline).（中文：R2-21 依旧成立——test-delta 从不经过containment咽喉点：它保留自己的私有 run()（spawnSync shell:true、env:buildRunEnv(process.env)）作为默认 exec，文件内没有任何 refuseUnsandboxedPhase/sandboxVerdict/containerised 引用，因此 required 下它会在基础侧以完整环境未沙箱化地重跑记录的失败套件并把归因作为证据发布。第二后果：PR 侧套件在受限容器环境里跑而基础侧在完整宿主环境里跑，环境敏感测试可能恰在一侧翻转，test-delta 会凭容器/宿主环境差异给 PR 制造伪 Critical（或放过真回归）。证据：第 2 轮探针——required+docker 在场，驱动 PR 自身代码的 runTestDelta，基础侧输出 'SECRET-IS:[hunter2-credential]'；已在 04a6ee1 复核：私有 spawnSync 未变、仍无门。修复方向：让 test-delta 的重跑经过 build-test 的 run()，或对 baseline 加同样的阶段顶部门。）
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file and the Windows-only case-folding path (R3-21) bite exactly there。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 15 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 16 条行内评论，其中 14 条是首次提出；上一轮发布了 22 条（其中 20 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/sandboxed-exec.ts（第 1、2 轮已出过发现，本轮又有 6 条）；packages/cli/src/commands/review/lib/sandboxed-exec.test.ts（第 2 轮已出过发现，本轮又有 5 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179,:315 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment choke point. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on exactly one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. witness: round-2 probe — QWEN_REVIEW_SANDBOX=required with answering docker, runTestDelta driven through the PR's own code: DELTA base-side output 'SECRET-IS:[hunter2-credential]' (the recorded repo command ran directly in the host shell with the full environment); re-verified at 04a6ee1: the private spawnSync is unchanged and the file still has no gate. Fix direction: route test-delta's rerun through build-test's run() (or add the same phase-top gate over the baseline).（中文：R2-21 依旧成立——test-delta 从不经过containment咽喉点：它保留自己的私有 run()（spawnSync shell:true、env:buildRunEnv(process.env)）作为默认 exec，文件内没有任何 refuseUnsandboxedPhase/sandboxVerdict/containerised 引用，因此 required 下它会在基础侧以完整环境未沙箱化地重跑记录的失败套件并把归因作为证据发布。第二后果：PR 侧套件在受限容器环境里跑而基础侧在完整宿主环境里跑，环境敏感测试可能恰在一侧翻转，test-delta 会凭容器/宿主环境差异给 PR 制造伪 Critical（或放过真回归）。证据：第 2 轮探针——required+docker 在场，驱动 PR 自身代码的 runTestDelta，基础侧输出 'SECRET-IS:[hunter2-credential]'；已在 04a6ee1 复核：私有 spawnSync 未变、仍无门。修复方向：让 test-delta 的重跑经过 build-test 的 run()，或对 baseline 加同样的阶段顶部门。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // reaches a spawn at all: a repo this adapter cannot scope is handed to the | ||
| // AGENT's own shell (`unsupportedReport`), which would otherwise run the | ||
| // install and the suite with nothing consulted. | ||
| const refusal = refuseUnsandboxedPhase(root); |
There was a problem hiding this comment.
[Critical] R1-1: the round-1/round-2 blocker's hand-off arm is still open at this head. Under required with a runtime ANSWERING and a mountable tree, a repo the npm adapter cannot scope (yarn/pnpm/bun — no package-lock.json — or no package.json) passes this gate (refuseUnsandboxedPhase returns null whenever the verdict is container and the tree is mountable), reaches unsupportedReport with ok: true, and the agent brief's unsupported bullet then instructs the agent to run the reviewed repo's install/build/test in its OWN shell with the full environment — the exact exposure #9556's design exists to remove, under the mode that promises it cannot happen. The gate's own comment below says it sits at phase top exactly because this route "would otherwise run the install and the suite with nothing consulted" — yet under required+runtime+mountable the gate consults nothing about scopeability. The round-3 increment fixed the no-runtime and unmountable arms; this arm remains.
witness: probe (real runtime answering, real mountRootFor, yarn fixture under a real .qwen/tmp layout):
policy in force: required | phase-top gate: null | adapter selected: true
report.toolchain: unsupported | report.ok: true
report.note: 'This is a yarn.lock repo ... Run yarn install --frozen-lockfile first, then fall back to the build/test precedence in your brief' — with exec stubbed to throw, never invoked
Fix: when the verdict is not direct, an unscopeable repo must produce a refusal-shaped report (toolchain: 'refused', ok: false), never an unsupported hand-off; alternatively widen refuseUnsandboxedPhase to model scopeability.
中文说明
R1-1：第 1/2 轮阻断项的交接臂在本 head 上仍然敞开。在 required 且运行时在场、树可挂载时，npm 适配器无法 scope 的仓库（yarn/pnpm/bun——没有 package-lock.json——或没有 package.json）会通过此门（只要判定为 container 且树可挂载，refuseUnsandboxedPhase 就返回 null），到达 ok: true 的 unsupportedReport，随后 agent 简报的 unsupported 条目指示 agent 在自己的 shell 里以完整环境运行被审仓库的 install/build/test——这正是 #9556 的设计要消除的暴露，发生在承诺不会发生的模式下。下方门自己的注释说它放在阶段顶部正是因为这条路径"否则会在什么都不查询的情况下运行 install 和套件"——可在 required+运行时在场+可挂载下，门对可 scope 性什么都没查。第 3 轮增量修掉了无运行时臂和不可挂载臂；此臂仍在。
证据：探针（真实运行时在场、真实 mountRootFor、真实 .qwen/tmp 布局下的 yarn 夹具）：生效策略 required | 阶段顶部门：null | 报告 toolchain: unsupported、ok: true，exec 被置为抛错且从未被调用。
修复：判定不为 direct 时，不可 scope 的仓库必须产出拒绝形态的报告（toolchain: 'refused'、ok: false），而不是 unsupported 交接；或让 refuseUnsandboxedPhase 把可 scope 性纳入判定。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // file one run rewrote, an ignored plant a sweep honoured. `--rm` is | ||
| // isolation by construction rather than by hygiene. | ||
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands at this head — deliberately deferred since round 1, mechanism unchanged, re-verified at this commit. The single RW bind mount covers the whole review temp dir, so it covers every pipeline tree's .git gitfile: containerized PR code can rewrite a gitfile to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge, and the host-side git invocations that follow the redirect (probe-tree creation, scratch trees) execute the planted filter as the reviewing user — host code execution. The module doc's "<repo>/.git stays outside it" is true of the common dir only, not of the per-tree gitfiles inside the mount; the doc correction committed to in round 1 (R2-9) is still absent.
中文说明
R1-3：在本 head 上依旧成立——自第 1 轮起被有意推迟，机制未变，已在本提交复核。唯一的 RW 绑定挂载覆盖整个 review 临时目录，因此覆盖每棵流水线树的 .git gitfile：容器化的 PR 代码可以把 gitfile 改写为 gitdir: <同一挂载内植入的目录> 并带上 filter.<x>.smudge，随后跟随该重定向的宿主侧 git 调用（探针树创建、scratch 树）会以审查用户身份执行植入的 filter——宿主代码执行。模块文档里"<repo>/.git 在挂载之外"只对 common dir 成立，对挂载内的各树 gitfile 不成立；第 1 轮承诺的文档更正（R2-9）仍未落地。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // report is indistinguishable from a contained run. The question the policy | ||
| // asks is whether THIS phase can be contained, and the mount is the half | ||
| // that can fail while the runtime is healthy. | ||
| if (mountRoot(root) === null) { |
There was a problem hiding this comment.
[Critical] This mount-failure refusal fires under auto too, not only required, and the reason hardcodes review.sandbox is "required". Under auto with a runtime answering, a /review of a local checkout (cwd not under .qwen/tmp) gets verdict container, this branch fires, and both consumers (build-test.ts:814, test-efficacy.ts:2496) refuse the whole phase — zero build/test evidence — blaming a policy the operator never set. That contradicts auto's documented semantics in this same module ("use a container when one is available, run directly when not"), containerised()'s docstring naming the local-checkout case as direct-run, and the asymmetry that auto with NO runtime runs directly with disclosure.
witness: probe through the real module:
verdict(auto, runtime answering) = {"kind":"container","runtime":"docker"}
refuseUnsandboxedPhase(localCheckout, autoVerdict) = 'review.sandbox is "required" and this tree cannot be mounted: ...'
asymmetry arm: verdict(auto, no runtime) = {"kind":"direct",...}; gate(localCheckout, that) = null
| if (mountRoot(root) === null) { | |
| if (policy === 'required' && mountRoot(root) === null) { |
(pass the policy in, and let the message name the policy actually in force; under auto fall through to the documented direct-with-disclosure path).
中文说明
这个挂载失败拒绝在 auto 下同样触发，而不只是 required，且理由文本硬编码了 review.sandbox is "required"。在 auto 且运行时在场时，对本地检出的 /review（cwd 不在 .qwen/tmp 之内）得到 container 判定，此分支触发，两个消费者（build-test.ts:814、test-efficacy.ts:2496）拒绝整个阶段——零构建/测试证据——并把责任推给操作者从未设置的策略。这与同一模块里 auto 的文档语义（"有容器则用容器，否则直接运行"）、containerised() 文档把本地检出列为直跑情形、以及"auto 无运行时反而直跑并披露"的不对称相矛盾。
证据：对真实模块的探针：verdict(auto, 运行时在场) = container；refuseUnsandboxedPhase(本地检出, auto 判定) 返回 'review.sandbox is "required" and this tree cannot be mounted...'；不对称臂：auto 无运行时为 direct，门返回 null。
修复：仅当策略为 required 时拒绝挂载失败（把策略传入），消息写明实际生效的策略；auto 落回文档化的"直跑+披露"路径。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // isolation by construction rather than by hygiene. | ||
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, | ||
| '--workdir', |
There was a problem hiding this comment.
[Critical] --workdir receives the lexical resolve(cwd) while this round's mountRootFor change makes the bind mount use realpathSync(root) (next line's opts.tmpDir). When any ancestor above the repo's parent is a symlink — the span redirectedAncestor deliberately stops before — the lexical workdir does not exist under the realpath'd mount inside the container. Both call sites pass lexical cwds (containerised: cwd: resolve(cwd); probeContainer: resolve(probeTree)). Regression introduced THIS round — the pre-round lexical mount matched the lexical workdir.
witness: fixture base/link -> base/real, repo under link/sub:
mountRootFor() = /tmp/r32.../real/sub/repo/.qwen/tmp
--workdir = /tmp/r32.../link/sub/repo/.qwen/tmp/review-pr-1
workdir inside mount? false | realpath(cwd) inside mount? true (the implied one-line fix flips it)
live docker arm (exact argv shape): docker auto-created the lexical path as an EMPTY root-owned dir
while the planted package.json is visible only under the real mount path
Consequence: every sandboxed npm ci/build/test runs in the wrong empty directory — fabricated failure evidence attributed to the PR, probes map inconclusive, and with --user uid:gid the mapped uid cannot even write the root-owned dir.
Fix: canonicalise on the same axis as the mount — realpath the cwd before handing it to containerCommand (verifying it stays under tmpDir), or mount the real root at the lexical path.
中文说明
--workdir 拿到的是词法 resolve(cwd)，而本轮 mountRootFor 的改动让绑定挂载改用 realpathSync(root)（下一行的 opts.tmpDir）。当仓库父目录之上的任何祖先是符号链接时——正是 redirectedAncestor 有意不检查的那段——词法 workdir 在容器内并不位于 realpath 化后的挂载之下。两个调用点都传词法 cwd（containerised 传 cwd: resolve(cwd)；probeContainer 传 resolve(probeTree)）。这是本轮引入的回归——改动前词法挂载与词法 workdir 是一致的。
证据：夹具 base/link -> base/real、仓库在 link/sub 下：mountRootFor() 返回 real 路径，--workdir 是 link 路径，workdir 不在挂载内（realpath(cwd) 在）；真实 docker 臂：docker 把词法路径自动创建为空的 root 属主目录，植入的 package.json 只在真实挂载路径下可见。
后果：每个容器化的 npm ci/构建/测试都跑在错误的空目录里——伪造的失败证据被记到 PR 头上，探针全部 inconclusive，且带 --user uid:gid 时映射用户连那个 root 属主目录都写不了。
修复：与挂载同轴规范化——把 cwd realpath 化后再交给 containerCommand（并校验仍在 tmpDir 之下），或把真实路径挂载到词法路径上。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| 'DOCKER_CONTEXT', | ||
| 'CONTAINER_HOST', |
There was a problem hiding this comment.
[Critical] The scrub misses the config-INDIRECTION daemon selectors: docker's DOCKER_CONFIG — and, at unit level, podman's CONTAINERS_CONF/CONTAINERS_REGISTRIES_CONF (podman steering not exercised — not installed on this runner). A repo-shipped .qwen/.env with DOCKER_CONFIG=.qwen/dockercfg plus committed config.json (currentContext) and contexts/meta/<sha>/meta.json (attacker endpoint) redirects the availability probe AND every docker run at an attacker-controlled daemon: required reads as satisfied, the RW mount spec is handed over, and all build/test/probe evidence is fabricated. loadEnvironment admits the key (it is in no exclusion list) and marks it file-sourced, but this loop deletes only its five listed keys.
witness: end-to-end probe (planted config dir + fake TLS daemon):
CONTROL (no DOCKER_CONFIG): docker info -> 29.1.3, exit=0; fake.log: (empty)
ATTACK (DOCKER_CONFIG=<planted>): docker info -> 99.9.9-fake, exit=0; fake.log: HEAD /_ping, GET /v1.48/info
docker run arm: fake.log received POST /v1.48/containers/create
unit probe: DOCKER_CONFIG/CONTAINERS_CONF/CONTAINERS_REGISTRIES_CONF all survive runtimeClientEnv();
adding them to this list flips the probe
| 'DOCKER_CONTEXT', | |
| 'CONTAINER_HOST', | |
| 'DOCKER_CONTEXT', | |
| 'CONTAINER_HOST', | |
| 'DOCKER_CONFIG', |
(and CONTAINERS_CONF / CONTAINERS_REGISTRIES_CONF for the podman arm — file-sourced-only deletion preserves an operator's genuine values).
中文说明
清除列表漏掉了配置间接类的守护进程选择器：docker 的 DOCKER_CONFIG——以及单元层面 podman 的 CONTAINERS_CONF/CONTAINERS_REGISTRIES_CONF（podman 的操纵未实测——本机未安装）。仓库自带的 .qwen/.env 写入 DOCKER_CONFIG=.qwen/dockercfg，加上提交的 config.json（currentContext）与 contexts/meta/<sha>/meta.json（攻击者端点），就能把可用性探测和每一次 docker run 重定向到攻击者控制的守护进程：required 读作已满足，RW 挂载规格被交出，全部构建/测试/探针证据被伪造。loadEnvironment 会接纳该键（不在任何排除清单）并标记为文件来源，但此循环只删除列出的五个键。
证据：端到端探针（植入配置目录+假 TLS 守护进程）：对照（无 DOCKER_CONFIG）docker info -> 29.1.3、fake.log 为空；攻击（植入 DOCKER_CONFIG）docker info -> 99.9.9-fake、fake.log 收到 HEAD /_ping、GET /v1.48/info；docker run 臂收到 POST /v1.48/containers/create。单元探针：三个间接键都穿过 runtimeClientEnv()；加入本列表即可翻转。
修复：把 DOCKER_CONFIG（以及 podman 的 CONTAINERS_CONF/CONTAINERS_REGISTRIES_CONF）加入此列表——仅删文件来源值，操作者自己的值保留。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const elsewhere = tmp(); | ||
| mkdirSync(join(elsewhere, 'tmp', 'review-pr-1'), { recursive: true }); | ||
| mkdirSync(join(root, '.qwen'), { recursive: true }); | ||
| symlinkSync(join(elsewhere, 'tmp'), join(root, '.qwen', 'tmp')); |
There was a problem hiding this comment.
[Suggestion] This suite plants the redirect at exactly one of the four positions the walk lstats (mount root, .qwen, repo root, repo's parent); a mutation shrinking the walk to the mount root alone ships all 18 tests green. The production shape the depth exists for: a PR committing .qwen itself as a mode-120000 symlink — a fresh clone materialises it, the pipeline's mkdirSync follows it and builds the layout at the link's target, and the shrunken walk then clears mountRootFor, handing a RW bind mount to the link's target (with .qwen → . that target is the clone root, pulling <clone>/.git into the mount — the widening this test's own comment says the guard exists to stop).
witness: mutation redirectedAncestor(root, root) [walk = mount root alone] -> suite 18 passed (18)
probe with the link planted at .qwen: mountRootFor -> '/tmp/r313-.../tmp' (mount proceeds) on the mutant; null on the real walk
Fix: add a case planting the symlink one level up (.qwen → an outside dir containing inner/tmp/review-pr-1) and expect null; optionally one more with the link at the repo-root component.
中文说明
本套件只在遍历会 lstat 的四个位置（挂载根、.qwen、仓库根、仓库父目录）中一个植入了重定向；把遍历收缩到仅挂载根的突变能让全部 18 个测试绿。这个深度存在的产品形态：PR 把 .qwen 本身提交为 mode 120000 的符号链接——新克隆会把它实体化，流水线的 mkdirSync 跟随它并在链接目标处建立布局，收缩后的遍历会让 mountRootFor 放行，把 RW 绑定挂载交给链接目标（.qwen → . 时目标就是克隆根，把 <clone>/.git 拉进挂载——正是本测试注释说该守卫要阻止的展宽）。
证据：突变（遍历仅剩挂载根）-> 套件 18 通过；链接植入在 .qwen 的探针：突变下 mountRootFor 返回被重定向的临时目录（挂载继续），真实遍历下为 null。
修复：补一个把符号链接植入上一层（.qwen → 含 inner/tmp/review-pr-1 的外部目录）并期望 null 的用例；可再加一个链接在仓库根组件的用例。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| isFileSourcedEnvKey(key) ? undefined : env[key]?.trim(); | ||
| return ( | ||
| pick('QWEN_REVIEW_SANDBOX_IMAGE') || | ||
| pick(CUSTOM_SANDBOX_IMAGE_ENV_VAR) || |
There was a problem hiding this comment.
[Suggestion] This second pick is untested in both directions — no test anywhere sets QWEN_CODE_CUSTOM_SANDBOX_IMAGE for reviewSandboxImage. Dropping the file-source guard from ONLY this branch ships green: a repo committing that key in .qwen/.env then still names the image the reviewed code executes in — the "the image IS the code" hole this round closed for QWEN_REVIEW_SANDBOX_IMAGE stays open one fallback down. (Dropping the pick entirely also ships green and silently loses the operator's qwen --sandbox image parity.)
witness: mutation (second branch unguarded): suite 18 passed (18)
Fix: add cases — the key set as a process variable is returned when QWEN_REVIEW_SANDBOX_IMAGE is absent; stubbed file-sourced via the spy, it falls through to the default.
中文说明
第二个取值分支双向都无测试——没有任何测试为 reviewSandboxImage 设置过 QWEN_CODE_CUSTOM_SANDBOX_IMAGE。只删掉这个分支的文件来源守卫即可绿通过：仓库在 .qwen/.env 里提交该键就仍能指定被审代码运行所用的镜像——本轮为 QWEN_REVIEW_SANDBOX_IMAGE 关上的"镜像即代码"漏洞在下一级回退上仍敞开。（整个删掉该取值同样绿，且悄悄丢掉操作者 qwen --sandbox 的镜像一致性。）
证据：突变（第二分支去守卫）：套件 18 通过。
修复：补用例——QWEN_REVIEW_SANDBOX_IMAGE 缺省时进程变量设置的该键被返回；经 spy 标记为文件来源时落回默认镜像。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // ...and the WIRING, not just `containerEnv` called with a literal: HOME | ||
| // must be the tmpfs the argv also declares, or the mapped uid has no | ||
| // writable home and npm fails before the install starts. | ||
| expect(passed).toContain(`HOME=${CONTAINER_HOME}`); |
There was a problem hiding this comment.
[Suggestion] The wiring test pins HOME against the argv but leaves the sibling npm_config_cache wiring unpinned — containerEnv is called directly with a literal in the array-equality test, never with the value containerCommand actually passes (join(opts.tmpDir, '.npm-cache')). Mutating that call argument to a path outside the mount ships 18/18 green: with the pinned --user uid:gid, npm cannot create a cache dir under root-owned / (EACCES on every sandboxed install), or the cache-in-mount property the module documents is silently lost.
witness: mutation containerEnv('/root/.npm-cache'): suite 18 passed
probe asserting the on-mount cache path flipped — Expected 'npm_config_cache=/repo/.qwen/tmp/.npm-cache', Received 'npm_config_cache=/root/.npm-cache'; pristine 19/19
Fix: extend this assertion so the cache is pinned to the mounted tmpDir:
| expect(passed).toContain(`HOME=${CONTAINER_HOME}`); | |
| expect(passed).toContain(`HOME=${CONTAINER_HOME}`); | |
| expect(passed).toContain(`npm_config_cache=${join(tmpDir, '.npm-cache')}`); |
中文说明
接线测试把 HOME 钉到了 argv 上，但兄弟 npm_config_cache 接线没钉——数组等价测试是用字面量直接调用 containerEnv，从不用 containerCommand 实际传入的值（join(opts.tmpDir, '.npm-cache')）。把那个实参突变为挂载外的路径，18/18 全绿：在已钉住的 --user uid:gid 下，npm 无法在 root 属主的 / 下创建缓存目录（每次容器化安装都 EACCES），或者模块文档所载的"缓存在挂载内"性质被悄悄丢掉。
证据：突变 containerEnv('/root/.npm-cache')：套件 18 通过；钉住挂载内缓存路径的探针翻转；原代码 19/19。
修复：扩展此断言——expect(passed).toContain(``npm_config_cache=${join(tmpDir, '.npm-cache')}``)，把缓存钉在挂载的 tmpDir 上。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * variable and not file-sourced. | ||
| */ | ||
| export function isFileSourcedEnvKey(key: string): boolean { | ||
| return dotEnvSourcedKeys.has(key) || settingsEnvSourcedKeys.has(key); |
There was a problem hiding this comment.
[Suggestion] This seam cannot distinguish OPERATOR-owned user-level .env files (~/.env, <globalQwenDir>/.env, legacy ~/.qwen/.env — findEnvFiles loads unconditionally, no trust gate) from repository content: the loader marks every key it writes with no file-scope distinction, and the three new security read sites treat "file-sourced" as "repository-sourced". An operator-set QWEN_REVIEW_SANDBOX=required in a user-level .env is silently dropped by sandboxPolicy and the policy degrades to the setting or off with no disclosure; the same marking scrubs an operator's DOCKER_HOST (colima/rootless) and QWEN_REVIEW_SANDBOX_IMAGE set there. Fails SAFE against the repository — silent operator degradation, not an exploit — but the module's own comment ("the operator's routes remain their settings file and their real shell environment") is narrower than the CLI's actual operator routes.
witness: real-loader probe, hermetic HOME, same values on two operator routes:
[arm A: shell export] policy = 'required'; DOCKER_HOST kept; image kept
[arm B: ~/.qwen/.env] loader wrote 'required', isFileSourcedEnvKey = true -> policy = 'off'; DOCKER_HOST scrubbed; image default
Fix: track file scope in the provenance sets (only repository-scoped files feed the security-decision set, or keep a separate home-sourced set); at minimum disclose on stderr when a file-sourced QWEN_REVIEW_SANDBOX is being ignored.
中文说明
这个接缝分不清操作者属主的用户级 .env 文件（~/.env、<globalQwenDir>/.env、遗留 ~/.qwen/.env——findEnvFiles 无条件加载、无信任关卡）与仓库内容：加载器对写入的每个键不做文件范围区分地打标，三个新的安全读取点把"文件来源"当作"仓库来源"。操作者在用户级 .env 里设置的 QWEN_REVIEW_SANDBOX=required 会被 sandboxPolicy 悄悄丢弃，策略降级为设置值或 off 且无任何披露；同一标记还会清掉操作者设在那里的 DOCKER_HOST（colima/rootless）与 QWEN_REVIEW_SANDBOX_IMAGE。对仓库方向是安全失败——是操作者意图的静默降级，不是可利用漏洞——但模块自己的注释（"操作者的途径是其设置文件和真实 shell 环境"）比 CLI 实际的操作者途径要窄。
证据：真实加载器探针、密封 HOME、两条操作者途径同值：[臂 A：shell 导出] 策略 'required'、DOCKER_HOST 保留；[臂 B：~/.qwen/.env] 加载器写入 'required' 且被标记文件来源 -> 策略 'off'、DOCKER_HOST 被清、镜像落回默认。
修复：在来源集合中记录文件范围（仅仓库范围文件进入安全决策集合，或单列家目录来源集合）；至少在文件来源的 QWEN_REVIEW_SANDBOX 被忽略时向 stderr 披露。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| cwd: join(tmpDir, 'review-pr-9'), | ||
| kind: 'install', | ||
| }); | ||
| const passed = args.filter((_, i) => args[i - 1] === '--env'); |
There was a problem hiding this comment.
[Suggestion] The canary inspects only the VALUES of the forwarded --env entries, never the SET: a regression to valueless --env KEY forwarding — which docker/podman resolve from the client process's own environment — ships the suite green while the inherited environment still crosses the boundary (--env OPENAI_API_KEY resolves from the client env into the reviewed code's postinstall). That is the natural shape of exactly the regression the adjacent comment says this test guards against.
witness: mutant (valueless forwarding of process.env): suite 18 passed (18)
mutant + set-pinning assertion: 1 failed
pristine + set-pinning: 18 passed
Fix: pin the full set — expect([...passed].sort()).toEqual([...containerEnv(join(tmpDir, '.npm-cache'))].sort()) — or additionally assert every entry matches /^[A-Za-z_][A-Za-z0-9_]*=/ so any valueless forward fails.
中文说明
金丝雀只检查转发 --env 条目的值，从不检查集合：退化为无值 --env KEY 转发——docker/podman 会从客户端进程自身环境解析——整套测试绿通过，而继承环境仍然越过边界（--env OPENAI_API_KEY 从客户端环境解析进被审代码的 postinstall）。这正是相邻注释声称本测试要防的退化的自然形态。
证据：突变（无值转发 process.env）：套件 18 通过；突变+集合钉住断言：1 失败；原代码+集合钉住：18 通过。
修复：钉住完整集合——expect([...passed].sort()).toEqual([...containerEnv(join(tmpDir, '.npm-cache'))].sort())——或再断言每个条目匹配 /^[A-Za-z_][A-Za-z0-9_]*=/，使任何无值转发失败。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…at their edges Round 3. Six Criticals, all of them real, and two were introduced by round 2's own fixes. - **The hand-off arm of the original blocker was still open.** The phase gate refuses when containment is impossible — but a repo the npm adapter cannot scope (yarn/pnpm/bun, no lockfile) reaches `unsupportedReport` with the gate satisfied: a runtime answered and the tree is mountable. That report tells the agent to install and build with its own shell, which nothing here contains. An inapplicable adapter is now a refusal under `required`, not a hand-off. - **The mount-failure refusal fired under `auto` too**, and its message hardcoded `required`. Under `auto` the contract is "contain it when that is possible", so an unmountable tree falls back to the direct spawn — refusing there would have taken build/test and efficacy evidence away from every local review the moment a daemon happened to be running. Mine, from round 2. - **`--workdir` got the lexical path while the mount got the realpath.** Round 2 made `mountRootFor` canonicalise; the workdir did not follow, so on any layout where the two spellings differ — `/var` against `/private/var` is the everyday one — the container was handed a directory it does not have and every command would fail before starting. Also mine, from round 2. - **The daemon scrub missed the indirection selectors.** `DOCKER_CONFIG`, `CONTAINERS_CONF` and friends name a config file that in turn names the daemon, the registries and the runtime: scrubbing the direct selectors and leaving these moves the same steering one level down. - **`isFileSourcedEnvKey` matched case-sensitively.** Windows env lookup is case-insensitive, so a `.env` committed as `docker_host=…` reaches the child exactly as `DOCKER_HOST` would while the exact-case test answers "not from a file" about a value that is. Same class as `sanitizedGitEnv`'s case fold. The gitfile finding stays deferred with its reasoning in the thread — it is not a hole this PR opens, and closing it belongs to the identity gates rather than to the sandbox. Two mutations came back green on the first pass and were the useful part of this round: the `DOCKER_CONFIG` scrub had no test because the fixture pinned one key rather than the set, and the operator's `SANDBOX_SET_UID_GID=false` opt-out had no test at all — both uid tests asserted the flag was PRESENT. The scrub test now asserts the whole set, and the opt-out has its own.
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-8 redirectedAncestor walk climbs one level higher than its siblings — already reported (comment 3837175064)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform arms and the win32 case-fold path bite exactly there.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.ts:161 — [review] D4-1 resetContainerRuntimeProbe has no consumers; the containerRuntime probe is untested
Convergence: round 4 posted 14 inline comment(s), 4 of them reported for the first time; the previous round posted 16 (14 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/sandboxed-exec.ts (findings in rounds 1, 3; 2 more now); packages/cli/src/commands/review/build-test.ts (findings in round 1; 1 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.)
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on exactly one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. witness: [probe] re-run at this head — QWEN_REVIEW_SANDBOX=required stubbed + PROBE_SECRET in env, real runTestDelta (default exec) reran the recorded failing npm test base-side in a fixture baseline whose test script echoes the secret: baseOutput "... SECRET-IS:[hunter2-credential] ...", baseExitCode 1 — the reviewed repo's code ran in the host shell with the full environment under the mode that forbids it. Fix direction: route test-delta's rerun through build-test's run() (or add the same phase-top gate over the baseline).
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform arms and the win32 case-fold path bite exactly there。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 14 条行内评论，其中 4 条是首次提出；上一轮发布了 16 条（其中 14 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/sandboxed-exec.ts（第 1、3 轮已出过发现，本轮又有 2 条）；packages/cli/src/commands/review/build-test.ts（第 1 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on exactly one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. witness: [probe] re-run at this head — QWEN_REVIEW_SANDBOX=required stubbed + PROBE_SECRET in env, real runTestDelta (default exec) reran the recorded failing npm test base-side in a fixture baseline whose test script echoes the secret: baseOutput "... SECRET-IS:[hunter2-credential] ...", baseExitCode 1 — the reviewed repo's code ran in the host shell with the full environment under the mode that forbids it. Fix direction: route test-delta's rerun through build-test's run() (or add the same phase-top gate over the baseline).
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // gate above passes whenever a runtime answers and the tree is mountable, | ||
| // which is exactly when a yarn/pnpm/bun repo still reaches this branch. So | ||
| // an inapplicable adapter is a refusal under that policy, not a hand-off. | ||
| if (!applicable && sandboxPolicy() === 'required') { |
There was a problem hiding this comment.
[Critical] R1-1: the round-4 gate added to close the hand-off arm is dead code — applicable is the readonly ReviewToolchainAdapter[] returned by selectToolchainAdapter, so !applicable is false for an empty and a non-empty array alike, and both unscopeable-repo routes still reach the toolchain: 'unsupported', ok: true hand-off under required. Witness: with QWEN_REVIEW_SANDBOX=required, an answering docker and a mountable tree, driving the real runBuildTest returns {"toolchain":"unsupported","ok":true} for both route A (no package.json — applicable is []) and route B (a yarn.lock repo where the adapter applies but concedes internally); patching the condition to applicable.length === 0 flips route A to refused/ok:false while route B stays unsupported — the npm adapter applies to yarn repos and concedes inside runNpmToolchain, so a length-only fix does not close the blocker's witness shape. The brief's unsupported rule then sends the agent to run the reviewed repository's install/build/test in its own shell with the full environment — the exact execution required forbids, reported as a clean hand-off. Fix by intercepting the outcome instead of the selection: under required, convert any returned report with toolchain === 'unsupported' into refusedReport(report.note), and throw rather than return on --resume, per the continuation invariant the adjacent branch states.
中文说明
R1-1：本轮为关闭交接臂新增的门是死代码——applicable 是 selectToolchainAdapter 返回的 readonly ReviewToolchainAdapter[]，数组永远为真值，空与非空时 !applicable 都是 false，因此在 required 下两条「适配器无法 scope」的路径仍会到达 toolchain: 'unsupported'、ok: true 的交接。证据：QWEN_REVIEW_SANDBOX=required、docker 在场、树可挂载时驱动真实 runBuildTest，路线 A（无 package.json——applicable 为 []）与路线 B（yarn.lock 仓库，适配器适用但在内部让步）都返回 {"toolchain":"unsupported","ok":true}；把条件改成 applicable.length === 0 后路线 A 翻转为 refused/ok:false，路线 B 仍是 unsupported——npm 适配器对 yarn 仓库适用、在 runNpmToolchain 内部让步，因此只改长度判断关不掉阻断项的证据形态。随后简报的 unsupported 规则会指示 agent 在自己的 shell 里以完整环境运行被审仓库的 install/build/test——正是 required 所禁止的执行，却被报告为一次干净的交接。修复请拦截结果而非选择：required 下把任何 toolchain === 'unsupported' 的返回报告转成 refusedReport(report.note)，并在 --resume 时抛错而不是返回，与相邻分支写明的续跑不变量一致。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // file one run rewrote, an ignored plant a sweep honoured. `--rm` is | ||
| // isolation by construction rather than by hygiene. | ||
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands at this head — deliberately deferred since round 1, mechanism unchanged, re-checked at this commit. The single RW bind mount covers the whole review temp dir, so it covers every pipeline tree's .git gitfile: containerized PR code can rewrite a gitfile to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge, and the host-side git invocations that follow the redirect (probe-tree restore, scratch-tree reset) give host code execution. The mount spec is byte-identical to what rounds 1–3 probed live, and this round's diff does not touch the mechanism. witness: not run this round — the round-1/round-2 live probes are recorded in the earlier threads and the code they exercised is unchanged. Either close this arm (mask each tree's gitfile from the writable surface, or make the host-side consumers refuse in-mount redirects) or hold it explicitly out of the PR's containment claims.
中文说明
R1-3：在本 head 上依旧成立——自第 1 轮起被有意推迟，机制未变，已在该提交复核。单一 RW 绑定挂载覆盖整个 review 临时目录，因此覆盖每棵流水线树的 .git gitfile：容器内的 PR 代码可以把 gitfile 改写为 gitdir: <同一挂载下的植入目录>，其中携带 filter.<x>.smudge，随后跟随该重定向的宿主侧 git 调用（探针树恢复、scratch 树复位）即形成宿主代码执行。挂载规格与第 1–3 轮活体探针所验证的代码逐字节一致，本轮 diff 未触碰该机制。证据：本轮未重跑——第 1/2 轮的活体探针记录在更早的讨论串中，其验证过的代码未变。要么关闭此臂（把各树 gitfile 从可写面上屏蔽，或让宿主侧消费者拒绝挂载内重定向），要么在 PR 的 containment 声明中明确把它排除在外。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| 'CONTAINERS_REGISTRIES_CONF', | ||
| 'CONTAINERS_STORAGE_CONF', | ||
| ]) { | ||
| if (isFileSourcedEnvKey(key)) delete scrubbed[key]; |
There was a problem hiding this comment.
[Critical] R3-4: only half-fixed — the case-fold added this round answers the query half (a case-variant committed key now reports file-sourced on Windows), but this delete remains exact-case on a plain object: the spread { ...env } carries the file's casing, so for a tracked docker_host the delete of the canonical DOCKER_HOST is a no-op and the value still crosses to the runtime client. Witness: driving the real (platform-free) delete half with a tracked case-variant key, the PR unmodified returns {"docker_host":"tcp://attacker:2375","keys":["PATH","docker_host"]} — the attacker value reaches the docker info probe and every boxed spawn — while deleting the tracked spelling flips it to {"keys":["PATH"]} (the win32 fold branch and Windows' case-insensitive child-env lookup are modeled, declared). A repo committing docker_host=tcp://attacker:2375 in .qwen/.env then steers the availability probe and every container spawn at a daemon it controls on Windows — required reads as satisfied and fabricated evidence comes back scored. Delete the tracked spelling, not the canonical one (on win32 iterate Object.keys(scrubbed) and match case-insensitively when isFileSourcedEnvKey holds — sibling precedent: sanitizedGitEnv in worktree.ts folds case for exactly this reason), and add a test seeding the tracker with a case-variant key.
中文说明
R3-4：只修了一半——本轮新增的大小写折叠解决了查询半边（Windows 上大小写变体的提交键现在会报告为文件来源），但这里的删除仍按规范拼写精确匹配普通对象：展开 { ...env } 携带的是文件里的大小写，因此对已追踪的 docker_host，删除规范形 DOCKER_HOST 是空操作，该值仍会进入运行时客户端。证据：用一个已追踪的大小写变体键驱动真实的（平台无关的）删除半边，未修改的 PR 返回 {"docker_host":"tcp://attacker:2375","keys":["PATH","docker_host"]}——攻击者值到达 docker info 探测与每一次容器化 spawn——改为删除被追踪的拼写后翻转为 {"keys":["PATH"]}（win32 折叠分支与 Windows 大小写不敏感的子进程环境查找为建模，已声明）。仓库在 .qwen/.env 提交 docker_host=tcp://attacker:2375 即可在 Windows 上把可用性探测与所有容器 spawn 指向其控制的守护进程——required 读作已满足，伪造证据被当作结果评分。请删除被追踪的拼写而不是规范拼写（win32 上遍历 Object.keys(scrubbed)，在 isFileSourcedEnvKey 成立时大小写不敏感匹配——先例：worktree.ts 的 sanitizedGitEnv 正是为此折叠大小写），并新增一个用大小写变体键填充追踪集合的测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| for (const entry of containerEnv(join(opts.tmpDir, '.npm-cache'))) { | ||
| args.push('--env', entry); | ||
| } | ||
| args.push(opts.image, 'sh', '-lc', command); |
There was a problem hiding this comment.
[Critical] A boxed run's timeout kills only the runtime client: the container has no identity (no --name, nothing captured) and there is no rm -f/kill anywhere in the pipeline — --rm fires only when the container exits on its own — so a suite that ignores SIGTERM persists with the mounted review temp dir still writable, past the budget and past the end of the review. Witness (live docker 29.1.3, the module's exact argv shape, a TERM-ignoring workload): BEFORE-KILL RUNNING: 8aeb1efdaab2 Up 3 seconds; kill -TERM on the client pid → the client is still alive; AFTER-KILL RUNNING: 8aeb1efdaab2 Up 6 seconds with a heartbeat file advancing inside the RW mount — on this docker version the attached client forwards/waits instead of dying, so spawnSync's deadline does not even return (--init does not help: the suite's own trap ignores the forwarded TERM). A PR committing process.on('SIGTERM', () => {}) in vitest.config.ts plus a suite that never finishes accumulates one orphan per malicious review on persistent runners — the review worktree other agents are reading, sibling probe trees and the shared npm cache stay writable by it. Give each container a unique --name and best-effort rm -f it on timeout/signal in both run() and runProbeSuite() — enforce the deadline at the daemon, not at the client.
中文说明
容器化运行的超时只会杀死运行时客户端：容器没有身份（无 --name，也未捕获任何 ID），整个流水线没有任何 rm -f/kill——--rm 只在容器自行退出时生效——因此一个忽略 SIGTERM 的套件会带着仍然可写的 review 临时目录挂载持续存在，超出预算、也超出审查结束时间。证据（真实 docker 29.1.3，模块的完整 argv 形态，忽略 TERM 的负载）：杀死前 RUNNING: 8aeb1efdaab2 Up 3 seconds；对客户端 pid 执行 kill -TERM → 客户端仍存活；杀死后 RUNNING: 8aeb1efdaab2 Up 6 seconds，RW 挂载内的心跳文件持续推进——该 docker 版本下附着客户端会转发/等待而不退出，spawnSync 的截止甚至不会返回（--init 无用：套件自己的 trap 忽略转发的 TERM）。PR 在 vitest.config.ts 提交 process.on('SIGTERM', () => {}) 加一个永不结束的套件，即可在持久 runner 上按恶意审查累积孤儿容器——其他 agent 正在读取的审查工作树、兄弟探针树与共享 npm 缓存都继续可写。请给每个容器唯一 --name，并在超时/信号时于 run() 与 runProbeSuite() 两处尽力 rm -f——截止应在守护进程层面执行，而不是客户端。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const suite = `node ${shellQuotePath( | ||
| findVitestBin(dependencyRoot), | ||
| )} run --reporter=json ${probes.map(shellQuotePath).join(' ')}`; | ||
| const boxed = probeContainer(suite, probeTree); |
There was a problem hiding this comment.
[Critical] Boxed probe runs mount the review temp dir in its CANONICAL (realpath'd) spelling — mountRootFor returns realpathSync(root), the mount and containerPathFor's --workdir use it — but every dependency-farm symlink the probe tree imports through keeps the LEXICAL spelling (exposeDependencies builds the targets from the un-canonicalised worktree argument). Whenever the checkout sits under a symlinked ancestor above redirectedAncestor's stop node — the macOS /tmp → /private/tmp case this module's own comments name — every farm link dangles inside the container and the containerised efficacy phase deterministically yields zero evidence. Witness (fixture alias → real, repo under alias/work/myrepo, real container): MOUNT …/real/work/myrepo/.qwen/tmp (canonical); farm target …/alias/…/node_modules/left-pad (lexical); with the PR's wiring the link DANGLES, and rebuilding the farm via exposeDependencies(probeTree, realpathSync(worktree)) makes it RESOLVE — the flip confirms the fix. As shipped, the baseline vitest run collects only import errors and mutants/hunks are skipped with "every file was red or collected nothing" — a wiring failure published as a statement about the PR's own suite, for every boxed review of such a checkout. Canonicalise what crosses the boundary (pass the realpath'd dependency root into exposeDependencies from the sandboxed path, or symlink to realpathSync(...) targets in farmNodeModules); scratch-tree shares the farm.
中文说明
容器化探针运行把 review 临时目录按其规范（realpath 化）拼写挂载——mountRootFor 返回 realpathSync(root)，挂载与 containerPathFor 的 --workdir 都用它——但探针树借以导入依赖的每条依赖 farm 符号链接仍保留词法拼写（exposeDependencies 用未规范化的 worktree 参数构造链接目标）。只要检出位于 redirectedAncestor 停止节点之上的符号链接祖先之下——正是本模块注释点名的 macOS /tmp → /private/tmp 常见情形——容器内每条 farm 链接都悬空，容器化的 efficacy 阶段确定性地零证据。证据（夹具 alias → real，仓库在 alias/work/myrepo 下，真实容器）：挂载 …/real/work/myrepo/.qwen/tmp（规范）；farm 目标 …/alias/…/node_modules/left-pad（词法）；按 PR 接线链接悬空，用 exposeDependencies(probeTree, realpathSync(worktree)) 重建 farm 后链接解析成功——翻转确认修复方向。按现状，基线 vitest 运行只收集到导入错误，mutant/hunk 以 "every file was red or collected nothing" 被跳过——把接线失败发布为对 PR 套件本身的论断，且对此类检出的每次容器化审查都如此。请把跨界的东西规范化（容器化路径向 exposeDependencies 传 realpath 化的依赖根，或在 farmNodeModules 里链接到 realpathSync(...) 目标）；scratch-tree 共用该 farm。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // ...and the WIRING, not just `containerEnv` called with a literal: HOME | ||
| // must be the tmpfs the argv also declares, or the mapped uid has no | ||
| // writable home and npm fails before the install starts. | ||
| expect(passed).toContain(`HOME=${CONTAINER_HOME}`); |
There was a problem hiding this comment.
[Suggestion] R3-12: still stands — the wiring test pins HOME against the argv but leaves the sibling npm_config_cache wiring unpinned: containerEnv is called directly with a literal in the array-equality test, never with the value containerCommand actually computes. A mutation dropping or misspelling the cache entry in the argv loop ships green — the container would re-download ~1 700 packages per review or write the cache outside the mount, surfacing as PR-attributed slowness/failure rather than a wiring error. Extend the assertion with the computed value: expect(passed).toContain(\npm_config_cache=${join(tmpDir, '.npm-cache')}`)`.
中文说明
R3-12：依旧成立——接线测试把 HOME 与 argv 对钉，却漏了兄弟 npm_config_cache 的接线：数组相等测试里是直接拿字面量调用 containerEnv，从不是 containerCommand 实际计算出的值。在 argv 循环里删掉或写错缓存条目的变异可以绿着上线——容器将每次审查重新下载约 1 700 个包，或把缓存写到挂载之外，最终以归咎于 PR 的慢/失败显现，而不是接线错误。请把断言扩展到计算值：expect(passed).toContain(\npm_config_cache=${join(tmpDir, '.npm-cache')}`)`。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * variable and not file-sourced. | ||
| */ | ||
| export function isFileSourcedEnvKey(key: string): boolean { | ||
| if (dotEnvSourcedKeys.has(key) || settingsEnvSourcedKeys.has(key)) { |
There was a problem hiding this comment.
[Suggestion] R3-13: still stands — this seam cannot distinguish OPERATOR-owned user-level .env files (~/.env, <globalQwenDir>/.env — loaded unconditionally, no trust gate) from repository content: both are marked file-sourced, so an operator's own docker_host / containment knobs kept in a user-level .env are scrubbed or ignored exactly like a repo-planted one — the daemon probe fails and auto silently degrades to direct runs (or required refuses), with nothing naming the cause. Record the provenance scope when populating the tracking sets (user-level vs repository-path .env) and answer file-sourced only for repository-reachable files, or document the over-marking as the contract where operators configure these values.
中文说明
R3-13：依旧成立——该接缝无法区分操作者自有的用户级 .env 文件（~/.env、<globalQwenDir>/.env——无条件加载、无信任门）与仓库内容：两者都被标记为文件来源，于是操作者自己放在用户级 .env 里的 docker_host/containment 旋钮会像仓库植入的一样被清除或忽略——守护进程探测失败，auto 悄悄降级为直跑（或 required 拒绝），且没有任何东西说明原因。请在填充追踪集合时记录来源作用域（用户级 vs 仓库路径的 .env），只对仓库可达的文件回答「文件来源」；或在操作者配置这些值的地方把这种过度标记写明为契约。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| kind: 'install', | ||
| }); | ||
| const passed = args.filter((_, i) => args[i - 1] === '--env'); | ||
| expect(passed.some((e) => e.includes('canary-should-not-cross'))).toBe( |
There was a problem hiding this comment.
[Suggestion] R3-14: still stands — the canary inspects only the VALUES of the forwarded --env entries, never the SET: the two forwarding forms docker/podman resolve from the client's own environment — bare --env OPENAI_API_KEY (no =value) and --env-file <dump> — cross the boundary without the canary string ever appearing in the argv this assertion inspects. A future edit forwarding process.env in docker's idiomatic passthrough form ships the suite green while OPENAI_API_KEY/GH_TOKEN from the CI reviewer's environment reach the reviewed repository's postinstall inside the container — the exact leak this module exists to prevent, re-opened with the guard reporting green. Pin the set, not the substring: expect(passed).toEqual(containerEnv(join(tmpDir, '.npm-cache'))) (any extra token — a bare key included — fails), plus expect(args).not.toContain('--env-file').
中文说明
R3-14：依旧成立——金丝雀只检查转发的 --env 条目的值，从不检查集合：docker/podman 会从客户端自身环境解析的两种转发形态——裸 --env OPENAI_API_KEY（无 =值）与 --env-file <dump>——都能穿过边界，而金丝雀字符串根本不会出现在该断言检查的 argv 里。未来某次按 docker 惯用透传形态转发 process.env 的修改会让套件绿着上线，同时 CI 审查者环境里的 OPENAI_API_KEY/GH_TOKEN 到达容器内被审仓库的 postinstall——正是本模块要防止的泄漏，且守卫报告为绿色地重新打开。请钉住集合而不是子串：expect(passed).toEqual(containerEnv(join(tmpDir, '.npm-cache')))（任何额外 token——包括裸键——都会失败），另加 expect(args).not.toContain('--env-file')。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| `to fill the gap.`, | ||
| }); | ||
| const refusal = refuseUnsandboxedPhase(root); | ||
| if (refusal && args.resume) { |
There was a problem hiding this comment.
[Suggestion] The round-4 wiring added here — the refuseUnsandboxedPhase gate, the refusedReport shape (toolchain: 'refused', ok: false), the --resume throw, and the boxed/direct selection in run() — is exercised by no test anywhere: build-test.test.ts (106 runBuildTest calls) has zero references to a sandbox policy, verdict, or refused, and sandboxed-exec.test.ts tests the decisions only with injected arguments. Every mutation that unwinds this round's fix ships green — flipping ok: false to true (the refusal reads as a clean hand-off), deleting the resume throw (a continuation overwrites the partial report with an identity-less refusal), or containerised() returning null unconditionally (every command silently runs direct under required). The dead !applicable condition one hunk down is exactly this class of defect and survived compile, lint and the full suite for the same reason. Pin the seam via vi.spyOn on the sandboxed-exec module: required + no runtime → refused, ok: false, exec never invoked; same with --resume → throws, --out untouched; required + container verdict + mountable → boxed argv; auto + no runtime → direct run unchanged.
中文说明
本轮在此新增的接线——refuseUnsandboxedPhase 门、refusedReport 形态（toolchain: 'refused'、ok: false）、--resume 抛错、以及 run() 里的容器化/直跑选择——没有任何测试覆盖：build-test.test.ts（106 次 runBuildTest 调用）对沙箱策略、判定、refused 零引用，sandboxed-exec.test.ts 只用注入参数测试这些决定。任何撤销本轮修复的变异都能绿着上线——把 ok: false 翻成 true（拒绝被读成干净交接）、删掉 resume 抛错（续跑会用无身份的拒绝报告覆盖部分报告）、或让 containerised() 无条件返回 null（required 下每条命令悄悄直跑）。下方 hunk 里那个死掉的 !applicable 条件正是这类缺陷，它通过编译、lint 和全套测试也是同一原因。请用 vi.spyOn 钉住该接缝：required + 无运行时 → refused、ok: false、exec 从未被调用；同样状态加 --resume → 抛错且 --out 不被改动；required + 容器判定 + 可挂载 → 容器化 argv；auto + 无运行时 → 直跑行为不变。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * then names a directory the container does not have, and every command | ||
| * fails before it starts. Null when the tree is not under a mountable root. | ||
| */ | ||
| export function containerPathFor(cwd: string): string | null { |
There was a problem hiding this comment.
[Suggestion] containerPathFor — added this round to decide the --workdir spelling that must match the realpath'd mount — is the only exported decision function the new test file does not import: its parent-fallback branch (leaf not yet created: canonicalise the parent, re-attach the leaf) has zero coverage. A mutation swapping or deleting the fallback leaves every test green, and a wrong spelling there makes every containerised command fail before it starts (--workdir names a directory the container lacks) — read in the report as the PR's build/test failure rather than wiring. Add cases: existing dir under a temp root (realpath), not-yet-created leaf falling back to the parent's realpath, and null when neither exists.
中文说明
containerPathFor——本轮新增、用于决定必须与 realpath 化挂载一致的 --workdir 拼写——是新测试文件唯一没有导入的导出决定函数：其父目录回退分支（叶子尚未创建：规范化父目录、重新拼接叶子）零覆盖。交换或删除该回退的变异可以让所有测试保持绿色，而那里的错误拼写会让每条容器化命令在启动前失败（--workdir 指向容器里不存在的目录）——报告会把这读成 PR 的构建/测试失败而不是接线问题。请补用例：临时根下的已存在目录（realpath）、尚未创建的叶子回退到父目录的 realpath、两者都不存在时返回 null。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Round 4. - **Last round's hand-off gate never ran.** It tested `!applicable`, and `applicable` is the filtered adapter ARRAY returned by `selectToolchainAdapter` — never falsy. The gate shipped green and closed nothing. It is judged on the RESULT now (`toolchain === 'unsupported'`), which also covers the second route to a hand-off — an adapter that applies and cannot scope, from inside the npm one — and the predicate is exported and tested rather than living inline where no test could see it. - **The boxed farm dangled under a symlinked ancestor.** Round 3 made the mount and `--workdir` canonical; `exposeDependencies` still built its link targets from the lexical root, so on the everyday macOS `/tmp` → `/private/tmp` layout every farm link resolved to a path the container does not have. The phase would then report "every file was red or collected nothing" — a wiring failure published as a statement about the PR's own suite. Canonicalised on the sandboxed path only; the direct path keeps the caller's spelling. - **A timed-out boxed run leaked its container.** `--rm` fires only on a self-exit, and the deadline kills the runtime CLIENT — so a suite whose own trap ignores the forwarded signal keeps running with the review temp dir writable, past the budget and past the end of the review. Containers get a unique `--name`, and both spawn sites `rm -f` it when the deadline fires. - **The daemon scrub deleted case-sensitively.** Round 3 taught `isFileSourcedEnvKey` to fold case on Windows and left the deletion exact — so a `docker_host` written by a repo `.env` was correctly detected and then not removed. Two mutations came back green again, and both were the round's real lesson: the hand-off refusal had no test (which is how its dead-code predecessor shipped), and the farm canonicalisation still has none — it needs a symlinked-ancestor fixture with a live runtime, which this machine cannot provide, and it is listed with the other integration gaps rather than claimed.
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 — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform arms and the win32 case-fold path bite exactly there.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.ts:160 — [probe] D5-1 containerRuntime probe is untested; both test seams are deadpackages/cli/src/commands/review/lib/sandboxed-exec.ts:122 — [probe] D5-2 settings half of sandboxPolicy matched case-sensitively ('Required' silently becomes off)packages/cli/src/commands/review/lib/sandboxed-exec.ts:176 — [review] D5-3 sandboxVerdict JSDoc documents the discarded first-cut 'returns direct' behaviourpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:45 — [probe] D5-4 strictest auto/required ordering unpinned (reorder mutant ships green)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:286 — [probe] D5-5 install network pinned only negatively (single-token --network=none mutant ships green)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:391 — [probe] D5-6 --init unpinned — deleting it ships green and degrades deadline enforcementpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:236 — [probe] D5-7 refuseUnsandboxedPhase PROCEED path unpinned (final return-null mutant ships green)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:350 — [probe] D5-8 --user default-on case unpinned (every test stubs SANDBOX_SET_UID_GID)packages/cli/src/commands/review/test-efficacy.ts:2536 — [review] D5-9 refusal note fires even when the phase had nothing to probepackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:209 — [probe] D5-10 auto-with-no-runtime → direct fallback untested (both mutants ship green)packages/cli/src/commands/review/build-test.ts:334 — [review] D5-11 run()'s doc comment orphaned above containerised()packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:180 — [probe] D5-12 SANDBOX-with-answering-probe direction unpinned (shortcut mutant ships green)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:42 — [probe] D5-13 env-side trim/toLowerCase normalization unpinnedpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:401 — [probe] D5-14 image slot in argv unpinned (image-dropped mutant ships green)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:269 — [probe] D5-15 podman runtime passthrough unpinned (hardcoded-docker mutant ships green)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:492 — [probe] D5-16 image override pick ordering unpinned (order-swap mutant ships green)
Convergence: round 5 posted 13 inline comment(s), 3 of them reported for the first time; the previous round posted 14 (4 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/sandboxed-exec.ts (findings in rounds 1, 3, 4; 1 more now); packages/cli/src/commands/review/build-test.ts (findings in rounds 1, 4; 1 more now); packages/cli/src/commands/review/test-efficacy.ts (findings in round 4; 1 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.)
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment choke point. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence, and the shipped agent brief (agent-briefs.ts:539) routes here whenever a test command failed. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. Witness: [probe] re-run at this head — real runTestDelta under QWEN_REVIEW_SANDBOX=required in a fixture baseline whose test script echoes a secret: PROBE-R221-BASE-OUTPUT "...SECRET-IS:[hunter2-credential]..." — the recorded repo command ran directly in the host shell with the full environment under the mode that forbids it. Fix direction: route test-delta's rerun through the same boundary — gate runTestDelta with refuseUnsandboxedPhase(baseline) (reporting the delta as unmeasured, like test-efficacy's refusal note) and/or execute through build-test's containerised run instead of the private direct-spawn copy.（中文：R2-21 依旧成立——test-delta 从不经过 containment 咽喉点：它保留私有 run()（spawnSync shell:true、env:buildRunEnv(process.env)）作为默认 exec，文件内没有任何 refuseUnsandboxedPhase/sandboxVerdict/containerised 引用，因此 required 下它会在基础侧以完整环境未沙箱化地重跑记录的失败套件并把归因作为证据发布，且内置简报在任一 test 命令失败时都会路由到这里。第二后果：PR 侧套件在受限容器环境里跑而基础侧在完整宿主环境里跑，环境敏感测试可能恰在一侧翻转，test-delta 会凭容器/宿主环境差异给 PR 制造伪 Critical（或放过真回归）。证据：本 head 上重跑探针——真实 runTestDelta、QWEN_REVIEW_SANDBOX=required、夹具基线的 test 脚本回显密钥：基础侧输出含 SECRET-IS:[hunter2-credential]。修复方向：让 test-delta 的重跑经过同一边界。）
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform arms and the win32 case-fold path bite exactly there。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 16 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 13 条行内评论，其中 3 条是首次提出；上一轮发布了 14 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/sandboxed-exec.ts（第 1、3、4 轮已出过发现，本轮又有 1 条）；packages/cli/src/commands/review/build-test.ts（第 1、4 轮已出过发现，本轮又有 1 条）；packages/cli/src/commands/review/test-efficacy.ts（第 4 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment choke point. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence, and the shipped agent brief (agent-briefs.ts:539) routes here whenever a test command failed. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. Witness: [probe] re-run at this head — real runTestDelta under QWEN_REVIEW_SANDBOX=required in a fixture baseline whose test script echoes a secret: PROBE-R221-BASE-OUTPUT "...SECRET-IS:[hunter2-credential]..." — the recorded repo command ran directly in the host shell with the full environment under the mode that forbids it. Fix direction: route test-delta's rerun through the same boundary — gate runTestDelta with refuseUnsandboxedPhase(baseline) (reporting the delta as unmeasured, like test-efficacy's refusal note) and/or execute through build-test's containerised run instead of the private direct-spawn copy.（中文：R2-21 依旧成立——test-delta 从不经过 containment 咽喉点：它保留私有 run()（spawnSync shell:true、env:buildRunEnv(process.env)）作为默认 exec，文件内没有任何 refuseUnsandboxedPhase/sandboxVerdict/containerised 引用，因此 required 下它会在基础侧以完整环境未沙箱化地重跑记录的失败套件并把归因作为证据发布，且内置简报在任一 test 命令失败时都会路由到这里。第二后果：PR 侧套件在受限容器环境里跑而基础侧在完整宿主环境里跑，环境敏感测试可能恰在一侧翻转，test-delta 会凭容器/宿主环境差异给 PR 制造伪 Critical（或放过真回归）。证据：本 head 上重跑探针——真实 runTestDelta、QWEN_REVIEW_SANDBOX=required、夹具基线的 test 脚本回显密钥：基础侧输出含 SECRET-IS:[hunter2-credential]。修复方向：让 test-delta 的重跑经过同一边界。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| return { | ||
| toolchain: 'unsupported', |
There was a problem hiding this comment.
[Critical] R1-1: still stands — the round-5 result-based gate wraps only two of the unsupported returns. This generic "No supported npm project here to scope" return in the !adapter branch bypasses refusedIfHandedOff, so under review.sandbox: required this route still hands the reviewed repo's commands to the agent's unsandboxed shell. With a runtime answering and a mountable tree, a repo with no root package.json (Python/Go/Rust, or a manifest in a subdirectory) returns {toolchain: 'unsupported', ok: true} with the note "Fall back to the build/test precedence in your brief — installing dependencies first", and the brief's unsupported rule (agent-briefs.ts:540) then sends the agent to run the reviewed repository's install/build/test in its own shell with the full environment — the exact execution required forbids, reported as a clean hand-off. Witness: real runBuildTest with a package.json-less fixture repo, docker answering, QWEN_REVIEW_SANDBOX=required — the bare return yields {"toolchain":"unsupported","ok":true}; wrapping it flips to {"toolchain":"refused","ok":false}.
| return { | |
| toolchain: 'unsupported', | |
| return refusedIfHandedOff({ | |
| toolchain: 'unsupported', |
(also wrap the applicable.length > 1 ambiguity return — unreachable with one adapter today, but it shares the shape — or funnel every unsupported outcome through one exit.)
中文说明
R1-1 依旧成立——第 5 轮基于结果的门只包住了两处 unsupported 返回。!adapter 分支里这个「此处没有可 scope 的 npm 项目」通用返回绕过了 refusedIfHandedOff，因此在 review.sandbox: required 下这条路径仍会把被审仓库的命令交给 agent 未沙箱化的 shell。运行时在场、树可挂载时，一个没有根 package.json 的仓库（Python/Go/Rust，或清单元文件在子目录里）会返回 {toolchain: 'unsupported', ok: true}，附注「回退到简报里的 build/test 优先顺序——先安装依赖」，随后简报的 unsupported 规则（agent-briefs.ts:540）会指示 agent 在自己的 shell 里以完整环境运行被审仓库的 install/build/test——正是 required 所禁止、却被报告为干净交接的执行。证据：对真实 runBuildTest、无 package.json 的夹具仓库、docker 在场、QWEN_REVIEW_SANDBOX=required——裸返回得到 {"toolchain":"unsupported","ok":true}；包上 refusedIfHandedOff 后翻转为 {"toolchain":"refused","ok":false}。请把 applicable.length > 1 的歧义返回也包上（今天只有一个适配器不可达，但形态相同），或让所有 unsupported 结果走同一个出口。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands — deliberately deferred since round 1, mechanism unchanged at this commit (this round's hunks do not touch it). The single RW bind mount covers the whole review temp dir, so it covers every pipeline tree's .git gitfile: containerized PR code can rewrite a gitfile to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge, and the host-side git invocations that follow the redirect (probe-tree restore, scratch-tree reset) give host code execution. Witness: not re-run this round — the round-1/round-2 live probes are recorded in the original threads, and the mount spec they exercised is byte-identical at this commit. Either close this arm (mask each tree's gitfile from the writable surface — e.g. a read-only bind over it — or make the host-side consumers refuse in-mount redirects), or hold it explicitly out of the PR's containment claims.
中文说明
R1-3 依旧成立——自第 1 轮起被有意推迟，机制在本提交上未变（本轮 hunk 未触碰它）。单一 RW 绑定挂载覆盖整个 review 临时目录，因此覆盖每棵流水线树的 .git gitfile：容器内的 PR 代码可以把 gitfile 改写为 gitdir: <同一挂载下的植入目录>，其中携带 filter.<x>.smudge，随后跟随该重定向的宿主侧 git 调用（探针树恢复、scratch 树复位）即形成宿主代码执行。证据：本轮未重跑——第 1/2 轮的活体探针记录在原始讨论串中，其验证过的挂载规格与本提交逐字节一致。要么关闭此臂（把各树 gitfile 从可写面上屏蔽——例如在其上叠加只读绑定——或让宿主侧消费者拒绝挂载内重定向），要么在 PR 的 containment 声明中明确把它排除在外。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const r = boxed | ||
| ? spawnSync(boxed.file, boxed.args, { | ||
| cwd: probeTree, |
There was a problem hiding this comment.
[Critical] R4-1: still stands with a narrowed mechanism — the round-5 --name + killContainer machinery is unreachable against a TERM-ignoring workload. spawnSync's timeout sends SIGTERM to the runtime CLIENT and then blocks until that client exits; the attached docker client forwards the signal and waits (this diff's own docker 29.1.3 measurement in containerCommand's --name comment). A container workload that ignores SIGTERM therefore hangs runProbeSuite forever, and this post-spawn killContainer never runs. The twin in build-test.ts's run() has the same shape. Witness (live docker 29.1.3, the module's exact argv): spawnSync('docker', ['run','--rm','--init','--name',…,'sh','-lc','trap "" TERM; sleep 20'], {timeout: 2500}) returned after 20444ms; the same spawn with killSignal: 'SIGKILL' returned at 2504ms, after which docker rm -f reaped the live container (STILL-ALIVE count 0). A PR committing a probe-reachable SIGTERM-ignore (e.g. in vitest globalSetup) plus a never-finishing suite hangs the review indefinitely and keeps the named container alive with the review temp dir mounted writable. Fix: pass killSignal: 'SIGKILL' in the boxed spawn options at both sites, so the client dies at the deadline, spawnSync returns, and the existing killContainer reaps the container through the daemon.
中文说明
R4-1 依旧成立，机制收窄——第 5 轮新增的 --name + killContainer 机制在忽略 TERM 的负载下不可达。spawnSync 的超时向运行时客户端发 SIGTERM，然后阻塞直到该客户端退出；附着的 docker 客户端会转发信号并等待（本 diff 在 containerCommand 的 --name 注释里对 docker 29.1.3 的实测正是如此）。因此忽略 SIGTERM 的容器负载会让 runProbeSuite 永久挂起，此处的 killContainer 永远不会执行。build-test.ts run() 里的孪生点同形。证据（真实 docker 29.1.3、模块的完整 argv）：带 timeout: 2500 的上述 spawnSync 20444ms 后才返回；改用 killSignal: 'SIGKILL' 后 2504ms 返回，随后 docker rm -f 清掉存活容器（STILL-ALIVE 计数 0）。PR 只需在探针可达处（如 vitest globalSetup）提交忽略 SIGTERM 的处理再加一个永不结束的套件，即可让审查无限挂起、命名容器带着仍可写的 review 临时目录挂载存活。修复：在两处容器分支的 spawn 选项里加 killSignal: 'SIGKILL'，使客户端在截止时刻被杀、spawnSync 返回，既有的 killContainer 再经守护进程清掉容器。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| 'DOCKER_CONFIG', | ||
| 'CONTAINERS_CONF', | ||
| 'CONTAINERS_REGISTRIES_CONF', | ||
| 'CONTAINERS_STORAGE_CONF', |
There was a problem hiding this comment.
[Critical] The scrub misses the proxy family: runtimeClientEnv drops the file-sourced daemon selectors but not HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXY, which docker/podman clients honour for the daemon connection — the function's own comment states the rule this violates ("Indirection counts as selection … scrubbing the direct selectors and leaving these would move the same steering one level down"). None of the loader's three exclusion tiers contains the proxy variables, so a committed .qwen/.env sets them and runtimeClientEnv() forwards them to the containerRuntime() probe and every boxed spawn. Witness (docker 29.1.3 against a fake proxy): ARM[http-proxy-nonloop] exit=0 proxy_requests=2 — proxy log HEAD .../_ping, GET .../v1.52/info — the client accepted the fake proxy's stubbed response as a successful docker info; controls noproxy/loopback had 0 proxy requests; loader probe: HTTP_PROXY: admitted=true fileSourced=true / forwarded=true while DOCKER_HOST is dropped. For an operator on a plaintext tcp:// daemon (the remote-engine shape the docstring names), required reads as satisfied against the attacker's proxy and fabricated evidence comes back scored; unix-socket/loopback daemons are unaffected. Note: for plaintext tcp:// the effective keys are HTTP_PROXY/http_proxy (HTTPS_PROXY covers TLS daemons).
| 'DOCKER_CONFIG', | |
| 'CONTAINERS_CONF', | |
| 'CONTAINERS_REGISTRIES_CONF', | |
| 'CONTAINERS_STORAGE_CONF', | |
| 'DOCKER_CONFIG', | |
| 'CONTAINERS_CONF', | |
| 'CONTAINERS_REGISTRIES_CONF', | |
| 'CONTAINERS_STORAGE_CONF', | |
| 'HTTP_PROXY', | |
| 'HTTPS_PROXY', | |
| 'ALL_PROXY', | |
| 'NO_PROXY', |
(file-sourced-only deletion preserves an operator's own shell-exported proxy, matching the DOCKER_HOST treatment.)
中文说明
清除列表漏掉了代理家族：runtimeClientEnv 删除文件来源的守护进程选择器，却不删 HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXY——docker/podman 客户端对守护进程连接同样认这些变量——本函数注释写明的规则正是它违反的（「间接也算选择……只清直接选择器而留下这些，等于把同样的操纵挪低一层」）。加载器的三层排除名单都不含代理变量，因此提交在 .qwen/.env 里的代理变量会被接纳，并经 runtimeClientEnv() 传给 containerRuntime() 探测与每一次容器化 spawn。证据（docker 29.1.3 对假代理）：ARM[http-proxy-nonloop] exit=0 proxy_requests=2——代理日志收到 HEAD .../_ping、GET .../v1.52/info——客户端把假代理的伪造响应当成成功的 docker info；对照（无代理／回环）代理请求为 0；加载器探针：HTTP_PROXY: admitted=true fileSourced=true / forwarded=true，而 DOCKER_HOST 被删除。对使用明文 tcp:// 守护进程的操作者（注释点名的远程引擎形态），required 会在攻击者代理面前读作已满足，伪造证据被当作结果评分；unix 套接字／回环守护进程不受影响。注意：明文 tcp:// 下生效的是 HTTP_PROXY/http_proxy（HTTPS_PROXY 覆盖 TLS 守护进程）。仅删文件来源值，操作者自己 shell 导出的代理保留，与 DOCKER_HOST 的处理一致。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(sandboxPolicy({}, {})).toBe('off'); | ||
| // A garbled value is not a policy — it falls through rather than being | ||
| // guessed at. | ||
| expect(sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'yes' }, {})).toBe('off'); |
There was a problem hiding this comment.
[Suggestion] R3-5: still stands — the file-sourced branch of sandboxPolicy (!fileSourced('QWEN_REVIEW_SANDBOX')) is never tested; no caller ever passes the injectable third parameter, and the env-can-only-tighten merge is equally unpinned in the distinguishing direction. Both mutations ship green against the real suite (21/21): deleting the clause (the real tracking set is empty in tests, so it is vacuous) and env-always-wins instead of strictest. Under the first, a committed <repo>/.qwen/.env decides the policy; under the second, an operator's required is silently downgraded by a stray QWEN_REVIEW_SANDBOX=off.
expect(
sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'required' }, {}, () => true),
).toBe('off');
expect(
sandboxPolicy({ QWEN_REVIEW_SANDBOX: 'off' }, { sandbox: 'required' }),
).toBe('required');中文说明
R3-5 依旧成立——sandboxPolicy 的文件来源分支（!fileSourced('QWEN_REVIEW_SANDBOX')）从未被测试；没有任何调用方传入可注入的第三个参数，而「环境只能收紧」的合并在判别方向上同样未被钉住。两个突变在真实套件下都绿（21/21）：删掉该子句（测试里真实追踪集为空，子句恒真），以及用「环境恒赢」替换 strictest。前者下提交在 <repo>/.qwen/.env 的值即可决定策略；后者下操作者的 required 会被一个偶然的 QWEN_REVIEW_SANDBOX=off 静默降级。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(passed.some((e) => e.includes('canary-should-not-cross'))).toBe( | ||
| false, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-14: still stands — the canary inspects only the VALUES of the forwarded --env entries, never the SET: a regression to valueless --env KEY forwarding ships the suite green, and docker/podman then resolve the value from the client's own environment. Leak verified against the live daemon: CANARY=secret docker run --rm --env CANARY alpine printenv CANARY printed the client-env value. Mutant (forward every process.env key valuelessly) ships 21/21 green; asserting every forwarded entry contains = flips it red.
for (const entry of passed) {
expect(entry).toContain('=');
}中文说明
R3-14 依旧成立——金丝雀只检查转发 --env 条目的值，从不检查集合：回归到无值 --env KEY 转发全套件绿，docker/podman 会从客户端自身环境解析该值。泄漏已在真实守护进程上验证：CANARY=secret docker run --rm --env CANARY alpine printenv CANARY 打印出客户端环境的值。突变（把 process.env 每个键无值转发）全套件绿（21/21）；断言每条转发条目都含 = 即可翻红。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const refusedReport = (why: string): BuildTestReport => ({ | ||
| toolchain: 'refused', |
There was a problem hiding this comment.
[Suggestion] R4-3: still stands — the wiring added in rounds 4-5 (the refuseUnsandboxedPhase gate, the refusedReport shape, the --resume throw, refusedIfHandedOff, and the boxed/direct selection in run()) is exercised by no test; build-test.test.ts has zero sandbox hits. The round-5 commit message itself records the failure class — "the hand-off refusal had no test (which is how its dead-code predecessor shipped)" — and this round's unwrapped generic return (R1-1) is the live instance. Add build-test-level tests: required + answering runtime + package.json-less root yields refused; a refused report on --resume throws and leaves the report file untouched; the boxed/direct selection follows the verdict.
中文说明
R4-3 依旧成立——第 4-5 轮新增的接线（refuseUnsandboxedPhase 门、refusedReport 形态、--resume 抛错、refusedIfHandedOff、run() 里的容器/直跑选择）没有任何测试覆盖；build-test.test.ts 里沙箱相关命中为零。第 5 轮提交信息自己记录了这一失败类——「交接拒绝没有测试（其死代码前辈正是这样上线的）」——而本轮未包裹的通用返回（R1-1）就是活例。请补 build-test 层测试：required + 运行时在场 + 无 package.json 的根得到 refused；对 refused 报告 --resume 抛错且不触碰报告文件；容器/直跑选择跟随判定。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * then names a directory the container does not have, and every command | ||
| * fails before it starts. Null when the tree is not under a mountable root. | ||
| */ | ||
| export function containerPathFor(cwd: string): string | null { |
There was a problem hiding this comment.
[Suggestion] R4-4: still stands — containerPathFor, which decides the --workdir spelling at both spawn sites and whose canonicalisation must agree with mountRootFor's realpath'd mount, is the only exported decision function the new test file does not import. Mutated to return the lexical resolve(cwd), the entire review suite stays green (4421 passed); on a host whose temp path resolves through a symlink (macOS /var → /private/var) the bind mount exists under the canonical path only, --workdir names a directory the container does not have, and every containerized command fails before it starts; a null-regression silently unsandboxes via containerised() returning null. Add tests alongside the mountRootFor describe using the same real-directory fixtures: symlinked-ancestor agreement and the not-yet-existing leaf fallback.
中文说明
R4-4 依旧成立——containerPathFor 决定两个 spawn 点的 --workdir 拼写、其规范化必须与 mountRootFor realpath 化的挂载一致，却是新测试文件唯一没有导入的导出决策函数。突变为返回词法 resolve(cwd) 后整个 review 套件仍绿（4421 通过）；在临时路径经符号链接解析的宿主上（macOS /var → /private/var），绑定挂载只存在于规范路径下，--workdir 指向容器里不存在的目录，每个容器化命令都会在启动前失败；返回 null 的回归则会经 containerised() 悄悄退回未沙箱化。请在 mountRootFor 的 describe 旁用同样的真实目录夹具补测：符号链接祖先下两者拼写一致，以及尚未创建的叶子回退。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // SIGTERM", which is a less useful sentence about the same event. The reason | ||
| // tag is derived from the whole result either way, so it does not depend on | ||
| // which message wins. | ||
| if (boxed && (r.error || r.signal)) { |
There was a problem hiding this comment.
[Suggestion] The spawn-level wiring added this round — boxed spawn with runtimeClientEnv() and this deadline/error→killContainer cleanup (and its build-test twin) — has no test at any level; the test file's header defers container startup to "an integration harness" this diff does not add. If this block regresses (deleted or inverted), no test turns red and the orphan container keeps the shared .qwen/tmp mount writable past the end of the review — the hazard the --name/killContainer machinery exists to close. Mock node:child_process.spawnSync to simulate a timed-out/errored boxed run and assert rm -f is aimed at the spawned container name; or land the deferred integration coverage and reference it in the header.
中文说明
本轮新增的 spawn 层接线——容器分支以 runtimeClientEnv() 为客户端环境、以及这处「截止/错误→killContainer」清理（连同 build-test 的孪生点）——在任何层面都没有测试；测试文件头部把容器启动推迟给一个本 diff 并未添加的「集成测试架」。若该块回归（被删或条件取反），没有任何测试变红，孤儿容器会在审查结束后继续以可写状态持有共享 .qwen/tmp 挂载——正是 --name/killContainer 机制要关闭的危险。请 mock node:child_process.spawnSync 模拟超时/出错的容器化运行，断言 rm -f 指向所启动容器的名字；或落地被推迟的集成覆盖并在头部注释中引用。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| env: buildRunEnv(), | ||
| }); | ||
| if (boxed && spawnTimedOut(r)) { |
There was a problem hiding this comment.
[Suggestion] The container-kill condition is asymmetric between the twins: this site kills only on spawnTimedOut(r), while test-efficacy's runProbeSuite kills on r.error || r.signal. Settled against the live daemon modeling this boxed run() exactly (same options, 70 MiB output from a TERM-ignoring container): spawn result {"errorCode":"ENOBUFS","signal":"SIGPIPE","status":null} — spawnTimedOut(r) is false, so killContainer never fires here while the --rm container keeps running with the mount writable; test-efficacy's twin condition kills it (rm -f reaped the orphan; it is a try/catch no-op when no container exists).
| if (boxed && spawnTimedOut(r)) { | |
| if (boxed && (spawnTimedOut(r) || r.error)) { |
中文说明
两处孪生的容器清理条件不对称：此处只在 spawnTimedOut(r) 时清理，而 test-efficacy 的 runProbeSuite 在 r.error || r.signal 时清理。按本处容器化 run() 的完整形态在真实守护进程上实测（相同选项、忽略 TERM 的容器输出 70 MiB）：spawn 结果 {"errorCode":"ENOBUFS","signal":"SIGPIPE","status":null}——spawnTimedOut(r) 为 false，此处的 killContainer 永不触发，--rm 容器带着可写挂载继续运行；孪生处的条件则能清掉它（rm -f 清除了孤儿容器；无容器时它是 try/catch 空操作）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…e reaper reachable Round 5. The first finding is the same one for the third time, and the third time is the one worth explaining: I had been guarding routes rather than the exit. - **Attempt one** tested `!applicable` — the filtered adapter ARRAY, never falsy — and was dead code. - **Attempt two** wrapped the two `adapter.run` returns and missed the `!adapter` branch's own `unsupported` report. - **Now** the conversion sits at the single place a report can reach a caller, and it is an exported `applyHandOffPolicy` rather than a branch inside a long function, so a test can reach it without a live container runtime. Both previous attempts failed the same way — a guard placed where no test could see it, in a function with more exits than the author was holding in mind. **The container reaper added last round was unreachable, not wrong.** `spawnSync` sends its `killSignal` at the deadline and then WAITS for the child to exit, so an attached runtime client that forwards SIGTERM to a workload whose own trap ignores it never returns — and the `killContainer` after it never runs. The boxed spawns use `killSignal: 'SIGKILL'` now: the client cannot ignore it, the call returns, and the container is reaped by name at the daemon, which is where the deadline had to be enforced. **The scrub missed the proxy family.** `HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY`/ `NO_PROXY` (and their lowercase spellings) are honoured by both clients for every daemon call, so a repo-shipped one interposes on the connection the direct selectors were scrubbed to protect. One test was removed rather than added: a `expect(killContainer).toBeTypeOf( 'function')` I wrote to "document" the SIGKILL reasoning. It asserts nothing and would have read as coverage; the reasoning belongs in the comment where it now lives alone. The `killSignal` choice is reasoned but not pinned — it needs a live runtime and a TERM-ignoring workload — and joins the integration gaps already listed in the PR body rather than being claimed.
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform arms (--user absence, win32 scrub fold) bite exactly there.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped.
Not explored to full depth (tool budget reached): chunk 2: executing npx vitest run src/commands/review/lib/sandboxed-exec.test.ts — the review worktree has no node_modules installed, and a full monorepo install exc….
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.ts:434 — [review] D6-1 containerCommand's doc block is attached to let containerSeq , leaving the function undocumentedpackages/cli/src/commands/review/lib/sandboxed-exec.ts:281 — [probe] D6-2 refuseUnsandboxedPhase's success exit (container+required+mountable → null) is pinned by no test; the refusal mutant ships green and the added case flips it redpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:117 — [probe] D6-3 the four lowercase proxy siblings in the scrub are unpinned and load-bearing on posix; the delete-mutant ships green and a file-sourced lowercase https_proxy sur…packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:336 — [probe] D6-4 the tmpfs assertion pins only the path prefix, not rw,mode=1777 ; ro/0755 mutants ship green and docker probes show HOME not writable by the mapped uidpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:83 — [probe] D6-5 the Windows side of the --user branch (flag correctly absent when getuid is undefined) is pinned by no assertion; the guard-drop mutant ships green on every platf…packages/cli/src/commands/review/build-test.ts:1109 — [probe] D6-6 the single-exit wrapper invocation runBuildTest = applyHandOffPolicy(...) is pinned by no test; dropping it ships 129/129 green and restores the hand-off defect under requir…packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:133 — [probe] D6-7 the win32 case-fold loop in runtimeClientEnv's drop() is pinned by no assertion; deleting it ships green and a committed docker_host variant survives on Windows
Convergence: round 6 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 13 (3 new). Findings keep coming back to the same files: packages/cli/src/commands/review/build-test.ts (findings in rounds 1, 4, 5; 1 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.)
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment boundary. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence, and the shipped agent brief (agent-briefs.ts:539) routes here whenever a test command failed. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. Witness: probe re-run at the round-5 head — real runTestDelta under QWEN_REVIEW_SANDBOX=required in a fixture baseline whose test script echoes a secret: base-side output '...SECRET-IS:[hunter2-credential]...' — the recorded repo command ran directly in the host shell with the full environment under the mode that forbids it; the file is unchanged at this head. Fix direction: route test-delta's rerun through the same boundary — gate runTestDelta with refuseUnsandboxedPhase (reporting the delta as unmeasured, like test-efficacy's refusal note) and/or execute through build-test's containerised run instead of the private direct-spawn copy.（中文：R2-21 依旧成立——test-delta 从不经过 containment 咽喉点：它保留私有 run()（spawnSync shell:true、env:buildRunEnv(process.env)）作为默认 exec，文件内没有任何 refuseUnsandboxedPhase/sandboxVerdict/containerised 引用，因此 required 下它会在基础侧以完整环境未沙箱化地重跑记录的失败套件并把归因作为证据发布，且内置简报在任一 test 命令失败时都会路由到这里。第二后果：PR 侧套件在受限容器环境里跑而基础侧在完整宿主环境里跑，环境敏感测试可能恰在一侧翻转，test-delta 会凭容器/宿主环境差异给 PR 制造伪 Critical（或放过真回归）。证据：探针在上一轮 head 上重跑——真实 runTestDelta、QWEN_REVIEW_SANDBOX=required、夹具基线的 test 脚本回显密钥：基础侧输出含 SECRET-IS:[hunter2-credential]；该文件在本 head 上未变。修复方向：让 test-delta 的重跑经过同一边界。）
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform arms (--user absence, win32 scrub fold) bite exactly there。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped。
未探索到全部深度（达到工具调用预算）：chunk 2：executing npx vitest run src/commands/review/lib/sandboxed-exec.test.ts — the review worktree has no node_modules installed, and a full monorepo install exc…。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 2 条行内评论，其中 1 条是首次提出；上一轮发布了 13 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/build-test.ts（第 1、4、5 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment boundary. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence, and the shipped agent brief (agent-briefs.ts:539) routes here whenever a test command failed. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. Witness: probe re-run at the round-5 head — real runTestDelta under QWEN_REVIEW_SANDBOX=required in a fixture baseline whose test script echoes a secret: base-side output '...SECRET-IS:[hunter2-credential]...' — the recorded repo command ran directly in the host shell with the full environment under the mode that forbids it; the file is unchanged at this head. Fix direction: route test-delta's rerun through the same boundary — gate runTestDelta with refuseUnsandboxedPhase (reporting the delta as unmeasured, like test-efficacy's refusal note) and/or execute through build-test's containerised run instead of the private direct-spawn copy.（中文：R2-21 依旧成立——test-delta 从不经过 containment 咽喉点：它保留私有 run()（spawnSync shell:true、env:buildRunEnv(process.env)）作为默认 exec，文件内没有任何 refuseUnsandboxedPhase/sandboxVerdict/containerised 引用，因此 required 下它会在基础侧以完整环境未沙箱化地重跑记录的失败套件并把归因作为证据发布，且内置简报在任一 test 命令失败时都会路由到这里。第二后果：PR 侧套件在受限容器环境里跑而基础侧在完整宿主环境里跑，环境敏感测试可能恰在一侧翻转，test-delta 会凭容器/宿主环境差异给 PR 制造伪 Critical（或放过真回归）。证据：探针在上一轮 head 上重跑——真实 runTestDelta、QWEN_REVIEW_SANDBOX=required、夹具基线的 test 脚本回显密钥：基础侧输出含 SECRET-IS:[hunter2-credential]；该文件在本 head 上未变。修复方向：让 test-delta 的重跑经过同一边界。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands at this head — deliberately deferred since round 1, mechanism unchanged; this round's hunks do not touch it. The single RW bind mount covers the whole review temp dir, so it covers every pipeline tree's .git gitfile: containerized PR code can rewrite a gitfile to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge, and the host-side git invocations that follow the redirect (probe-tree creation, restoreProbeTreeTracked after every sandboxed suite run) give host code execution. Rounds 1-2 reproduced this end-to-end (HOST-SIDE EXECUTION OBSERVED for both the restore checkout and git worktree add; scratch-tree's --git-common-dir identity gate refused the same tree); the mount spec is byte-identical at this commit. Either close this arm (mask each tree's gitfile from the writable surface — e.g. a read-only bind over it — or make the host-side consumers verify every tree's .git still resolves to its own admin entry under <repo>/.git/worktrees/ and refuse in-mount redirects), or hold it explicitly out of the PR's containment claims.
中文说明
R1-3 在本 head 上依旧成立——自第 1 轮起被有意推迟，机制未变，本轮 hunk 未触碰它。单一 RW 绑定挂载覆盖整个 review 临时目录，因此覆盖每棵流水线树的 .git gitfile：容器内的 PR 代码可以把 gitfile 改写为 gitdir: <同一挂载下的植入目录>，其中携带 filter.<x>.smudge，随后跟随该重定向的宿主侧 git 调用（探针树创建、每次沙箱化套件运行后的 restoreProbeTreeTracked）即形成宿主代码执行。第 1/2 轮已端到端复现（restore checkout 与 git worktree add 均观测到宿主侧执行；scratch-tree 的 --git-common-dir 身份关卡对同一棵树会拒绝）；挂载规格与本提交逐字节一致。要么关闭此臂（把各树 gitfile 从可写面上屏蔽——例如在其上叠加只读绑定——或让宿主侧消费者校验各树 .git 仍解析到 <repo>/.git/worktrees/ 下自己的 admin entry 并拒绝挂载内重定向），要么在 PR 的 containment 声明中明确把它排除在外。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| export function runBuildTest(args: BuildTestArgs): BuildTestReport { | ||
| return applyHandOffPolicy(runBuildTestUnguarded(args)); | ||
| } |
There was a problem hiding this comment.
[Critical] R6-2: the new single-exit wrapper also runs on --resume answers — a resumed report whose toolchain is 'unsupported' under a now-required policy is replaced by a fresh identity-less refusedReport, and the handler's unconditional writeFileSync(args.out, …) destroys the report the call was asked to continue; every later --resume fails the identity check ('records no run identity'), so the chain is permanently dead. The diff's own invariant ('A continuation must never answer with a FRESH report') is enforced by throws on the other two continuation routes (refusal gate, !adapter) but not at this third exit. Verified end-to-end against the real runBuildTest with an answering docker: call 1 (policy off, yarn-shaped repo) writes an identity-stamped unsupported hand-off; call 2 (--resume, QWEN_REVIEW_SANDBOX=required) returns toolchain=refused, run=null — original overwritten; call 3 throws 'records no run identity'. The throw-on-resume arm preserves the report. Trigger: the policy is read per call, so a tightening between call 1 and the resume (env or operator setting) plus an answering runtime on the resume call is enough; the repo shape is the common unscopeable case (yarn/pnpm/bun, no package-lock.json).
| export function runBuildTest(args: BuildTestArgs): BuildTestReport { | |
| return applyHandOffPolicy(runBuildTestUnguarded(args)); | |
| } | |
| export function runBuildTest(args: BuildTestArgs): BuildTestReport { | |
| const report = runBuildTestUnguarded(args); | |
| if (args.resume && handOffRefused(report.toolchain, sandboxPolicy())) { | |
| throw new Error( | |
| `refusing to continue this run: converting the resumed hand-off to a ` + | |
| `refusal would replace the report at ${args.out} with a fresh one ` + | |
| `that records no run identity. Re-run without --resume under the ` + | |
| `new policy.`, | |
| ); | |
| } | |
| return applyHandOffPolicy(report); | |
| } |
中文说明
R6-2：新的单出口包装器对 --resume 的应答同样生效——在策略已变为 required 时，一份 toolchain 为 'unsupported' 的续跑报告会被替换成一份没有运行身份的全新 refusedReport，而处理器无条件的 writeFileSync(args.out, …) 会毁掉这次调用本应续跑的那份报告；之后每一次 --resume 都会撞上身份检查（'records no run identity'），续跑链被永久杀死。本 diff 自己的不变量（"续跑绝不能以一份全新报告作答"）在另外两条续跑路径（拒绝门、!adapter）上以抛错落实，唯独这第三个出口没有。已对真实 runBuildTest（docker 在场）端到端验证：第 1 次调用（策略 off、yarn 形态仓库）写下带运行身份的 unsupported 交接报告；第 2 次调用（--resume、QWEN_REVIEW_SANDBOX=required）返回 toolchain=refused, run=null——原报告被覆盖；第 3 次调用抛 'records no run identity'。改为续跑时抛错后报告得以保留。触发条件：策略按调用读取，因此第 1 次调用与续跑之间收紧策略（环境变量或操作者设置）、且续跑时运行时应答即可；仓库形态正是最常见的不可 scope 情形（yarn/pnpm/bun、无 package-lock.json）。修复见上方建议代码块。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…continuing Round 6, two Criticals: the deferred gitfile one, and this. Round 5's single-exit conversion runs on `--resume` answers too. A resumed report whose toolchain is `unsupported`, under a policy that tightened between the first call and the continuation, was replaced by a fresh refusal — which the handler writes unconditionally, over the report the call was asked to continue. That refusal carries no run identity, so every later `--resume` fails the identity check and the round redoes install, build and every suite. "A continuation must never answer with a FRESH report" is enforced by a throw at the refusal gate and at `!adapter`. This conversion was added after both and did not have it. It does now. The trigger is ordinary rather than adversarial: the policy is read per call, so an operator raising it — or a workflow's `env:` — between call one and the resume is enough, on exactly the unscopeable repo shapes (yarn/pnpm/bun) that reach a hand-off at all. **The first test I wrote for this passed without the fix.** It drove `runBuildTest` with an incomplete argument object and asserted `.toThrow()`; the throw it saw was `--timeout must be a finite number of seconds`, from validation long before the code under test. A mutation caught it. Making the fixture faithful means reproducing the whole resume-identity apparatus, so the decision is a predicate instead — `resumeWouldDestroyReport`, tested directly, with the wiring left as one visible line. Same shape as round 5's `applyHandOffPolicy`, and for the same reason: a guard a test cannot reach is a guard that has already been wrong twice here.
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the sandboxed-exec platform arms (--user absence, win32 scrub fold) bite exactly there.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/build-test.ts:1143 — [review] the new resume-guard throw and single-exit conversion are unpinned at the runBuildTest levelpackages/cli/src/commands/review/build-test.ts:1086 — [probe] resumeWouldDestroyReport's insertion orphaned applyHandOffPolicy's doc commentpackages/cli/src/commands/review/build-test.ts:1144 — [review] the new throw message lacks the build-test: prefix every sibling error carriespackages/cli/src/commands/review/build-test.ts:433 — [probe] the timeout-reap wiring is unpinned — deleting the guard ships the suite greenpackages/cli/src/commands/review/build-test.ts:899 — [probe] the runBuildTest suite depends on the host machine's review.sandbox settingpackages/cli/src/commands/review/build-test.ts:399 — [probe] the first sandboxed command burns its deadline pulling the image
Convergence: round 7 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (1 new). Findings keep coming back to the same files: packages/cli/src/commands/review/build-test.ts (findings in round 6; 1 more now). 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 keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
[Critical] R1-3 (packages/cli/src/commands/review/lib/sandboxed-exec.ts:485 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — deliberately deferred since round 1, mechanism unchanged. The single RW bind mount covers every pipeline tree's .git gitfile: containerized PR code can rewrite a gitfile to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge, and the host-side git invocations that follow the redirect (probe-tree creation, restoreProbeTreeTracked after every sandboxed suite run) give host code execution. Witness: rounds 1-2 end-to-end reproduction — HOST-SIDE EXECUTION OBSERVED for both the restore checkout and git worktree add; scratch-tree's identity gate refused the same tree; the mount spec is byte-identical at this commit. Fix direction: mask each tree's gitfile from the writable surface (e.g. a read-only bind over it), or make the host-side consumers verify every tree's .git still resolves to its own admin entry under /.git/worktrees/ and refuse in-mount redirects — or land the documented follow-up before auto/required are turned on in CI.
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment boundary. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. Witness: probe re-run under QWEN_REVIEW_SANDBOX=required in a fixture baseline whose test script echoes a secret — base-side output '...SECRET-IS:[hunter2-credential]...'; the file is unchanged at this head and was re-discovered independently by this round's reverse audit. Fix direction: gate runTestDelta with refuseUnsandboxedPhase(baseline) (reporting the delta as unmeasured, like test-efficacy's refusal note) and/or execute through build-test's exported run() (its new optional kind defaults to the restrictive 'test').
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the sandboxed-exec platform arms (--user absence, win32 scrub fold) bite exactly there。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/build-test.ts（第 6 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R1-3 (packages/cli/src/commands/review/lib/sandboxed-exec.ts:485 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — deliberately deferred since round 1, mechanism unchanged. The single RW bind mount covers every pipeline tree's .git gitfile: containerized PR code can rewrite a gitfile to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge, and the host-side git invocations that follow the redirect (probe-tree creation, restoreProbeTreeTracked after every sandboxed suite run) give host code execution. Witness: rounds 1-2 end-to-end reproduction — HOST-SIDE EXECUTION OBSERVED for both the restore checkout and git worktree add; scratch-tree's identity gate refused the same tree; the mount spec is byte-identical at this commit. Fix direction: mask each tree's gitfile from the writable surface (e.g. a read-only bind over it), or make the host-side consumers verify every tree's .git still resolves to its own admin entry under /.git/worktrees/ and refuse in-mount redirects — or land the documented follow-up before auto/required are turned on in CI.
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment boundary. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. Witness: probe re-run under QWEN_REVIEW_SANDBOX=required in a fixture baseline whose test script echoes a secret — base-side output '...SECRET-IS:[hunter2-credential]...'; the file is unchanged at this head and was re-discovered independently by this round's reverse audit. Fix direction: gate runTestDelta with refuseUnsandboxedPhase(baseline) (reporting the delta as unmeasured, like test-efficacy's refusal note) and/or execute through build-test's exported run() (its new optional kind defaults to the restrictive 'test').
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // The deadline killed the CLIENT; the container outlives it — see the | ||
| // `--name` comment in `containerCommand`. Reach the daemon instead, then | ||
| // report the timeout exactly as before. | ||
| killContainer(boxed.runtime, boxed.name); |
There was a problem hiding this comment.
[Critical] R7-1: the boxed container is reaped only when the spawn TIMED OUT. A spawnSync maxBuffer overflow — the reviewed command writing more than 64 MB to one stdout/stderr stream, trivially produced by a postinstall/build/test — kills the runtime client with error.code === 'ENOBUFS', and spawnTimedOut() returns false for that shape, so killContainer is skipped. The container — holding the RW bind mount of the review temp dir — outlives the per-command deadline and the end of the review: --rm fires only on exit and nothing else stops it, so a workload that never exits leaves one orphan per malicious review on a persistent runner, still mutating the trees later phases read. The command is also misreported as an ordinary failure (timedOut: false, exitCode: null), with no hint that a live container still holds the mount.
witness: live probe with the module's exact argv shape and a 70 MB workload through the 64 MiB maxBuffer (Node v22.22.0, docker 24.0.9):
spawn result: {"errCode":"ENOBUFS","signal":"SIGKILL","status":null}
spawnTimedOut => false => killContainer called? false
docker ps after ENOBUFS kill of client: "qwen-review-verify-c1-45245 Up 31 seconds"
orphan container alive: true
The sibling site in test-efficacy.ts already reaps on the broader r.error || r.signal condition; this site checks only the timeout. Reap whenever the boxed client did not exit normally (status === null covers ETIMEDOUT, ENOBUFS and signal kills; for a client that never spawned, docker rm -f on the absent name fails silently by killContainer's try/catch construction):
if (boxed && r.status === null) {
// The deadline (or a buffer overflow) killed the CLIENT; the container
// outlives it. Reach the daemon instead.
killContainer(boxed.runtime, boxed.name);
}中文说明
容器化运行只有在 spawn 超时时才会被收割。spawnSync 的 maxBuffer 溢出——被审命令向单个 stdout/stderr 流写入超过 64 MB，一个 postinstall/构建/测试即可轻易制造——会以 error.code === 'ENOBUFS' 杀死运行时客户端，而 spawnTimedOut() 对这种形态返回 false，于是 killContainer 被跳过。容器带着 review 临时目录的 RW 绑定挂载存活过每命令截止、也存活过审查结束：--rm 只在自行退出时触发，没有任何别的机制停掉它——一个永不退出的负载会在持久 runner 上按恶意审查累积一个孤儿容器，继续改写后续阶段读取的树。该命令还会被误报为普通失败（timedOut: false、exitCode: null），丝毫不提示仍有活容器握着挂载。
证据：按模块 argv 形态、70 MB 负载穿过 64 MiB maxBuffer 的活体探针（Node v22.22.0、docker 24.0.9）：spawn 结果 {"errCode":"ENOBUFS","signal":"SIGKILL","status":null}；spawnTimedOut => false；killContainer 未被调用；客户端被杀后 docker ps 仍显示容器 Up 31 seconds——孤儿容器存活：true。
test-efficacy.ts 的孪生点已按更宽的 r.error || r.signal 条件收割，本处却只判超时。请在容器化客户端未正常退出时一律收割（status === null 覆盖 ETIMEDOUT、ENOBUFS 与信号杀；对从未成功 spawn 的客户端，按 killContainer 的 try/catch 构造，docker rm -f 一个不存在的名字会静默失败）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…timeout Round 7, one Critical. `build-test` reaped the boxed container when `spawnTimedOut(r)`, which is true for ETIMEDOUT and false for a `maxBuffer` overflow — and a reviewed command writing 64 MB to one stream is a postinstall away. The client dies with ENOBUFS, the reap is skipped, and the container keeps the review temp dir mounted read-write past the per-command deadline and past the end of the review. The sibling in `test-efficacy` already reaped on the broader `r.error || r.signal`. That the two had drifted to different conditions is how one came to miss a case the other caught, so they now share one exported predicate: `status === null` — exactly "the client did not exit normally", covering ETIMEDOUT, ENOBUFS and signal kills in one condition rather than a list of causes to keep in sync. A normal exit needs no reaping (`--rm` has fired), and a client that never spawned has no container, where the reap is a silent no-op by `killContainer`'s construction.
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the sandboxed-exec platform arms (--user absence, win32 scrub fold) bite exactly there.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped.
Test Plan (not a blocker): 324 passed — this review observed 23009 passed.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:129 (+10 locations) — [probe] under-pinned guarantees in the new test file: each named mutation ships the suite green (case-fold scrub, proxy entries, image slot, cache wiring, tmp…packages/cli/src/commands/review/build-test.ts:900 (+3 locations) — [probe] sandbox wiring in the consumer phases is unpinned: gate deletions ship green and containment silently disappearspackages/cli/src/commands/review/build-test.ts:1087 (+4 locations) — [review] doc blocks displaced from the declarations they describe by this diff's insertionspackages/cli/src/commands/review/lib/sandboxed-exec.ts:174 — [review] sandboxVerdict's docstring documents the rejected first-cut behavior (SANDBOX-set 'returns direct'), the implementation and test pin the oppositepackages/cli/src/commands/review/lib/sandboxed-exec.ts:160 — [review] dead seam: resetContainerRuntimeProbe has no caller and no caller ever passes force=true to containerRuntimepackages/cli/src/commands/review/build-test.ts:434 — [probe] the reap keys on status === null only: a client that exits WITH a status while the container keeps running (measured: stream-loss exit 125) is never reaped — orphan container hold…packages/cli/src/commands/review/lib/sandboxed-exec.ts:122 — [probe] sandboxPolicy's settings value is not normalized (env half trims/lowercases): a near-miss review.sandbox like "Required" silently resolves to off — fail-open on the centra…packages/cli/src/commands/review/lib/sandboxed-exec.ts:273 — [review] the required refusal message names one cause for mountRootFor === null; symlink-redirected/unresolvable trees get a false, unactionable reasonpackages/cli/src/commands/review/lib/sandboxed-exec.ts:502 — [probe] --volume colon interpolation: a colon-containing checkout path makes every containerized command fail (invalid spec: too many colons) although the probe passes and auto ne…
Convergence: round 8 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment boundary. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. Witness: probe re-run under QWEN_REVIEW_SANDBOX=required in a fixture baseline whose test script echoes a secret — base-side output '...SECRET-IS:[hunter2-credential]...'; the file is unchanged at this head (verified at HEAD this round). Fix direction: gate runTestDelta with refuseUnsandboxedPhase(baseline) (reporting the delta as unmeasured, like test-efficacy's refusal note) and/or execute through build-test's exported run() (its new optional kind defaults to the restrictive 'test').
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the sandboxed-exec platform arms (--user absence, win32 scrub fold) bite exactly there。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the uid/opt-out and symlink-path arms are platform-shaped。
Test Plan（非阻断）：324 passed — this review observed 23009 passed。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 9 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 8 轮发布了 2 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R2-21 (packages/cli/src/commands/review/test-delta.ts:179 — file not in this diff, so this blocker cannot be anchored inline): still stands at this head — test-delta never crosses the containment boundary. It keeps its own private run() (spawnSync(command, { shell: true, env: buildRunEnv(process.env) })) as the default exec — zero references to refuseUnsandboxedPhase/sandboxVerdict/containerised in the file — so under required it reruns the recorded failing suites BASE-SIDE unsandboxed with the full environment and publishes the attribution as evidence. Second consequence: PR-side suites run in the restricted container env while the base side runs in the full host env, so an env-sensitive test can flip on one side and test-delta manufactures a Critical against the PR (or dismisses a real regression) out of a container/host env difference. Witness: probe re-run under QWEN_REVIEW_SANDBOX=required in a fixture baseline whose test script echoes a secret — base-side output '...SECRET-IS:[hunter2-credential]...'; the file is unchanged at this head (verified at HEAD this round). Fix direction: gate runTestDelta with refuseUnsandboxedPhase(baseline) (reporting the delta as unmeasured, like test-efficacy's refusal note) and/or execute through build-test's exported run() (its new optional kind defaults to the restrictive 'test').
— qwen3.8-max via Qwen Code /review (v0.22.0)
| for (const key of [ | ||
| 'DOCKER_HOST', | ||
| 'DOCKER_CERT_PATH', |
There was a problem hiding this comment.
[Critical] runtimeClientEnv's scrub list misses DOCKER_API_VERSION, so a repository-shipped value survives the scrub and silently switches auto containment off. A reviewed PR that commits DOCKER_API_VERSION=1.9 in .qwen/.env has it admitted by the loader (qwen-scoped file, no exclusion list names it) and tracked file-sourced; the scrub does not drop it, so containerRuntime's docker info probe runs with it and exits 1 ("client version 1.9 is too old"); the probe answers null, and under review.sandbox: auto the verdict becomes direct — every install/build/test/probe then executes the reviewed repository's own commands unsandboxed with the full process.env while the operator believes containment is active, and the fallback is silent because nothing consumes the verdict's disclose string. Under required the same input fails closed (refused), so the exposure is the auto posture — and a repo-induced probe failure is exactly the steering class this scrub list exists to prevent.
witness (A/B probe against the real loader + scrub + docker 24.0.9):
clean repo -> containerRuntime "docker", verdictAuto "container"
repo ships DOCKER_API_VERSION=1.9 -> fileSourced true, clientEnvKeepsDAV "1.9",
containerRuntime null, verdictAuto "direct",
verdictRequired "refused"
DOCKER_API_VERSION=1.9 docker info -> exit 1, "client version 1.9 is too old"
key added to the scrub list -> verdictAuto "container" restored
| for (const key of [ | |
| 'DOCKER_HOST', | |
| 'DOCKER_CERT_PATH', | |
| for (const key of [ | |
| 'DOCKER_HOST', | |
| 'DOCKER_CERT_PATH', | |
| 'DOCKER_API_VERSION', |
中文说明
[Critical] runtimeClientEnv 的清洗列表漏掉了 DOCKER_API_VERSION，仓库内置的值可以穿过清洗、静默关掉 auto 封装。被审 PR 在 .qwen/.env 里提交 DOCKER_API_VERSION=1.9 时，loader 会接受它（qwen 作用域文件，没有任何排除列表列它）并记为文件来源；清洗不丢弃它，于是 containerRuntime 的 docker info 探测带着它运行、以退出码 1 结束（"client version 1.9 is too old"）；探测答 null，review.sandbox: auto 下判定变为 direct——每一次 install/build/test/探针都以完整 process.env 在沙箱外执行被审仓库自己的命令，而操作者以为封装生效，且该回退是无声的（判定里的 disclose 字符串没有任何消费者）。required 下同样输入会失败关闭（refused），因此暴露面是 auto 姿态——而「仓库诱导探测失败」正是这份清洗列表要防的转向类。
证据（对真实 loader + 清洗 + docker 24.0.9 的 A/B 探针）：干净仓库 → containerRuntime "docker"、verdictAuto "container"；仓库内置 DOCKER_API_VERSION=1.9 → fileSourced true、clientEnvKeepsDAV "1.9"、containerRuntime null、verdictAuto "direct"、verdictRequired "refused"；DOCKER_API_VERSION=1.9 docker info → 退出码 1、"client version 1.9 is too old"；把该键加入清洗列表 → verdictAuto "container" 恢复。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands at this head — deliberately deferred since round 1, mechanism unchanged (this round's diff includes this file but does not touch the mount geometry). The single RW bind mount covers every pipeline tree's .git gitfile: it sits inside its tree, i.e. inside this mount, and containerized PR code can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge; the host-side git invocations that follow the redirect (probe-tree creation, restoreProbeTreeTracked after every sandboxed suite run) then give host code execution. The PR description discloses this class under "What this does not close" (in-worktree contamination, host-side git operations), so this is re-posted as a carried blocker for the record rather than a new discovery — it remains the open boundary between this PR's containment and the host.
witness: rounds 1-2 end-to-end reproduction — HOST-SIDE EXECUTION OBSERVED for both the restore checkout and git worktree add; scratch-tree's identity gate refused the same tree; the mount spec is byte-identical at this commit.
Fix direction: mask each tree's gitfile from the writable surface (e.g. a read-only bind over it), or make the host-side consumers verify every tree's .git still resolves to its own admin entry under <repo>/.git/worktrees/ and refuse in-mount redirects — or land the documented follow-up before auto/required are turned on in CI.
中文说明
[Critical] R1-3：在本 head 上依旧成立——自第 1 轮起被有意延后，机制未变（本轮 diff 包含本文件，但未触碰挂载几何）。单一 RW 绑定挂载覆盖每棵流水线树的 .git gitfile：gitfile 位于树内、即挂载之内，容器化的 PR 代码可以把它改写为 gitdir: <同一挂载下种植的目录>，其中携带 filter.<x>.smudge；随后跟随该重定向的宿主侧 git 调用（探针树创建、每次沙箱化套件运行后的 restoreProbeTreeTracked）即获得宿主机代码执行。PR 描述已在「本 PR 关不掉的」一节披露该类（工作树内部污染、宿主侧 git 操作），因此本条作为携带的阻断项重新发布以存档，而非新发现——它仍是本 PR 的封装与宿主之间未关闭的边界。
证据：第 1-2 轮端到端复现——恢复检出与 git worktree add 两条路径均观察到宿主侧执行；scratch-tree 的身份门拒绝了同一棵树；本提交上挂载规格逐字节相同。
修复方向：把各树的 gitfile 从可写面中遮掉（例如在其上加只读绑定挂载），或让宿主侧消费者校验每棵树的 .git 仍解析到 <repo>/.git/worktrees/ 下自己的管理条目并拒绝挂载内重定向——或在 auto/required 于 CI 开启前先落地已记录的后续工作。
— qwen3.8-max via Qwen Code /review (v0.22.0)
The scrub list named the daemon selectors, then the proxy family, and still missed DOCKER_API_VERSION — which selects no daemon at all. It just makes every client call fail, and under `sandbox: auto` a failing availability probe reads as "no runtime here", so a repository that ships one value in its .env turns containment off without a word. The class has no last entry. Replace the list with the rule it was trying to approximate: a key the loader wrote from a file the reviewed checkout supplies does not reach the process that decides whether containment happened. The client needs no repository-provided environment for anything, and the workload's environment is an allowlist either way. Deleting restores rather than approximates: the loader records a key as file-sourced only where the real environment had nothing, so a file value never shadows an inherited one and the client keeps the operator's own DOCKER_HOST, PATH and HOME.
commented
Aug 23, 2026
第 8 轮
|
commented
Aug 24, 2026
第 14 轮
唯一的延后项确实是我上一轮编辑留下的：把用例包进 R1-3判断和状态与上一轮相同：机制我不争，等的是维护者对范围的决定，不是等实现。 review 套件 99 文件 4437 通过 / 2 跳过。tsc / lint / prettier 干净。 |
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite was not run on Windows locally — the changed test file's platform arms (skipIf gating, win32-only mount pin) run there.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite was not run on macOS locally — the changed test file's realpath/mount fixtures run there.
Test Plan (not a blocker): 324 passed — this review observed 23018 passed.
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:58 — [probe] sandboxPolicy's file-sourced guard and tighten-only property are unpinned (two surviving mutants)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:258 — [probe] refuseUnsandboxedPhase's pass path (container + required + mountable → final return null) is never executed by any testpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:234 — [probe] sandboxVerdict's auto-with-no-runtime → direct fallback return is unreached by any assertionpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:25 — [probe] containerPathFor (feeds --workdir at both spawn sites) has zero tests; realpath and parent-fallback mutants ship greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:459 — [probe] the --tmpfs assertion pins only the path prefix; the load-bearing mode=1777 is unpinnedpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:231 — [probe] the auto-with-runtime → container verdict is never observed directly (null under auto cannot tell container from direct)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:639 — [probe] the symlinked-ANCESTOR half of redirectedAncestor is exercised by no test (only a link AT .qwen/tmp is planted)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:394 — [probe] the '.git stays outside' property inspects only the FIRST --volume; a second mount ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:76 — [probe] QWEN_CODE_CUSTOM_SANDBOX_IMAGE has no provenance test; unguard and delete-fallback mutants ship greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:129 — [probe] the provenance-scrub fixture is satisfiable by a 16-key name-blocklist mutantpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:204 — [probe] the SANDBOX-set + runtime-ANSWERING verdict cell is unpinned (runtime-conditioned shortcut mutant ships green)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:458 — [probe] nothing asserts npm_config_cache lines up with the mount; an off-mount cache mutant ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:550 — [probe] killContainer (the exported reaper) has zero tests; rm-without--f and no-op mutants ship green, failed reap silentpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:219 — [probe] containerRuntime (sandboxVerdict's default probe at every real call site) is referenced by no testpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:519 — [probe] --init is deliberately emitted by containerCommand but mentioned by no assertionpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:376 — [probe] the capitalised "Rootless" negative twin is never exercised; a value-blind matcher mutant ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:686 — [probe] image-override precedence is unpinned when both keys are set; an operand-swap mutant ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:60 — [probe] the garbled-value fall-through contract is unpinned for the SETTINGS layer (a typo behaves like auto)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:248 — [probe] no test reads the container verdict's runtime payload; hardcoded/swapped-runtime mutants ship green
[Critical] R1-3: still stands at this head — deliberately deferred by the author since round 1 (reaffirmed in the round-10 and round-14 replies; the round-15 delta is test-comment-only and does not touch the mechanism). The single RW bind mount covers the whole review temp dir (containerCommand mounts tmpDir:tmpDir RW at sandboxed-exec.ts:616-617, read directly at the reviewed commit), so it covers every pipeline tree's .git gitfile: the gitfile sits inside its tree, i.e. inside this mount, and containerized PR code can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge; the host-side git invocations that follow the redirect (probe-tree restore, base-tree rerun, discard-worktree sweeps) then execute the planted filter — host code execution out of the container. Round 13's live probe executed the planted smudge filter on the host; no filter/fsmonitor/replace neutralisation has landed on the restore/creation paths since (only scratch-tree carries the identity gate). Until the gitfiles sit outside the RW mount (or the filter surface is otherwise neutralised), required cannot claim to contain host execution.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite was not run on Windows locally — the changed test file's platform arms (skipIf gating, win32-only mount pin) run there。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite was not run on macOS locally — the changed test file's realpath/mount fixtures run there。
Test Plan（非阻断）：324 passed — this review observed 23018 passed。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改：共 19 条（原文未翻译，列表见上方英文部分）。
[Critical] R1-3: still stands at this head — deliberately deferred by the author since round 1 (reaffirmed in the round-10 and round-14 replies; the round-15 delta is test-comment-only and does not touch the mechanism). The single RW bind mount covers the whole review temp dir (containerCommand mounts tmpDir:tmpDir RW at sandboxed-exec.ts:616-617, read directly at the reviewed commit), so it covers every pipeline tree's .git gitfile: the gitfile sits inside its tree, i.e. inside this mount, and containerized PR code can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge; the host-side git invocations that follow the redirect (probe-tree restore, base-tree rerun, discard-worktree sweeps) then execute the planted filter — host code execution out of the container. Round 13's live probe executed the planted smudge filter on the host; no filter/fsmonitor/replace neutralisation has landed on the restore/creation paths since (only scratch-tree carries the identity gate). Until the gitfiles sit outside the RW mount (or the filter surface is otherwise neutralised), required cannot claim to contain host execution.
— qwen3.8-max via Qwen Code /review (v0.22.0)
Four cells the suite reached only by accident of the machine it ran on, each carrying a live mutant on a property the PR description states as a guarantee. All four functions already took their ambient dependency as an injectable parameter, so this is assertions, not seams. - sandboxPolicy: strictest-wins in BOTH directions, and a file-sourced value counting for nothing. "A repository cannot switch off the containment that exists to contain it" was described in a comment and asserted nowhere; env-overrides-settings, a looser strictest, and a dropped file-sourced guard all shipped green. - sandboxVerdict: `auto` with nothing answering runs DIRECTLY. Every other case here either has a runtime or is `required`, so a mutant refusing instead — turning `auto` into `required` on every machine without docker — survived. - refuseUnsandboxedPhase: the PASS path. Every other assertion about this gate is a refusal, so unconditional refusal under `required` — every review on a perfectly good host — survived. - containerPathFor: it feeds `--workdir` at both spawn sites, and had no test at all. A lexical spelling names a directory the container does not have; the parent fallback is what lets a probe tree be named before it is built. Mutation-checked, one at a time: seven mutants, seven reds.
commented
Aug 24, 2026
第 15 轮
这四条延后项虽然被你标成建议级，但我认为它们比我前几轮修的某些东西更要紧，所以全部处理了——它们是本 PR 描述里当作保证写出来的性质上存活的变异。四个函数的环境依赖本来就是可注入参数，所以这次加的是断言，不是接缝。
逐条变异，七个变异，七次红。 R1-3状态与前几轮相同：机制我不争，等的是维护者对范围的决定。 review 套件 99 文件 4441 通过 / 2 跳过。tsc / lint / prettier 干净。 |
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the new test file's platform-gated assertions (uid/symlink) bite exactly there.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform-gated assertions (win32/symlink privilege) bite exactly there.
Not explored to full depth (tool budget reached): "agent 2": run sandboxed-exec.test.ts under vitest to confirm it is green (no node_modules/dist in this worktree; full install+build exceeded remaining budget). Assertion-….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 10.
Test Plan (not a blocker): 324 passed — this review observed 23018 passed.
Deferred under the convergence posture (round 16, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:58 — [review] sandboxPolicy's settings = operatorReviewSettings() default (every production decision shape) is never discriminated — mutant settings = {} ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:61 — [review] QWEN_REVIEW_SANDBOX .trim().toLowerCase() normalisation pinned by nothing — QWEN_REVIEW_SANDBOX=Required would silently fall through to offpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:171 — [review] SANDBOX_SET_UID_GID parse normalisation unpinned — SANDBOX_SET_UID_GID=False silently ignores the documented operator opt-outpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:178 — [review] containerCommand options literal pasted twice (uid A/B pair) plus a third near-identical base fixture — the pair can silently drift apartpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:203 — [review] SANDBOX-set AND runtime-answering verdict cell unpinned — mutant returning direct ships green, losing required's secret-stripping inside qwen --sandboxpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:220 — [review] refuseUnsandboxedPhase default-parameter wiring (the shape of all three production gates) never observed — default mutants turn refusals into passespackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:265 — [review] sandboxVerdict's env = process.env default (every production call shape) never discriminated — disclosure strings wrong on every production callpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:304 — [review] file-sourced-ignore gate's no-settings shape untested — a repo .env could flip an operator-less review to requiredpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:317 — [review] auto/off-with-runtime verdict cells unpinned — under auto on a daemon host the reviewed code would silently run with the full environmentpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:318 — [review] auto-fallback disclosure ternary ('ran directly' vs 'does not strip its environment') has no assertion — branch swap ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:321 — [review] sandboxVerdict's probe = containerRuntime default (all three production call sites) unobserved — probe = () => null mutant ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:354 — [review] containerPathFor test leaks its mkdtemp tree in os.tmpdir() on every run — sibling mountRootFor block sweeps its fixtures, this one doesn'tpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:367 — [review] containerPathFor's third answer — null from the inner catch — never reached by any test — lexical --workdir instead of the direct fallbackpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:440 — [review] containerRuntime — the availability probe every sandboxVerdict turns on — has zero tests; inverting its success condition ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:440 — [review] runtimeIsRootless default reader wiring ( read = cachedInfoDocument , both production call sites) never observed — rootless hosts answer rootfulpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:497 — [review] file: opts.runtime pass-through pinned only against 'docker' — a hardcoded-'docker' mutant ships green and ENOENTs every podman hostpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:555 — [review] env canary inspects only the --env long spelling — -e / --env-file forwarding re-introduction escapes every assertionpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:555 — [review] valueless --env KEY pass-through crosses the canary (the value resolves from the client env, never appears in argv) — pin the whole token surfacepackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:562 — [review] npm_config_cache allowlist wiring (the one entry composed at the call site) is never asserted — cache-off-mount mutant ships greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:563 — [review] HOME tmpfs writability ( rw,mode=1777 ) unpinned — a :ro regression ships green and every contained npm ci fails before it starts- …and 4 more (see the run report)
[Critical] R1-3 (packages/cli/src/commands/review/lib/sandboxed-exec.ts:616 — file not in this round's diff, so this blocker cannot be anchored inline): still stands at this head — deliberately deferred by the author since round 1 (reaffirmed in the round-10 and round-14 replies); the mechanism is unchanged and the round-16 delta (test-only, sandboxed-exec.test.ts) does not touch it. The single RW bind mount covers the whole review temp dir (containerCommand mounts tmpDir:tmpDir RW at sandboxed-exec.ts:616-617, read directly at the reviewed commit), so it covers every pipeline tree's .git gitfile: the gitfile sits inside its tree, i.e. inside this mount, and containerized PR code can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge; the host-side git invocations that follow the redirect (probe-tree restore, base-tree rerun, discard-worktree sweeps) then execute the planted filter — host code execution out of the container. Round 13's live probe executed the planted smudge filter on the host (canary present; honest-gitfile control clean); no filter/fsmonitor/replace neutralisation has landed on the restore/creation paths since (only scratch-tree carries the identity gate). Until the gitfiles sit outside the RW mount (or the filter surface is otherwise neutralised), required cannot claim to contain host execution.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on macOS locally — the new test file's platform-gated assertions (uid/symlink) bite exactly there。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the packages/cli suite did not run on Windows locally — the new test file's platform-gated assertions (win32/symlink privilege) bite exactly there。
未探索到全部深度（达到工具调用预算）："agent 2"：run sandboxed-exec.test.ts under vitest to confirm it is green (no node_modules/dist in this worktree; full install+build exceeded remaining budget). Assertion-…。
未审查：反向审计——在 10 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：324 passed — this review observed 23018 passed。
收敛姿态下延后（第 16 轮，非阻断）——已记录，本轮不要求修改：共 24 条（原文未翻译，列表见上方英文部分）。
[Critical] R1-3 (packages/cli/src/commands/review/lib/sandboxed-exec.ts:616 — file not in this round's diff, so this blocker cannot be anchored inline): still stands at this head — deliberately deferred by the author since round 1 (reaffirmed in the round-10 and round-14 replies); the mechanism is unchanged and the round-16 delta (test-only, sandboxed-exec.test.ts) does not touch it. The single RW bind mount covers the whole review temp dir (containerCommand mounts tmpDir:tmpDir RW at sandboxed-exec.ts:616-617, read directly at the reviewed commit), so it covers every pipeline tree's .git gitfile: the gitfile sits inside its tree, i.e. inside this mount, and containerized PR code can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge; the host-side git invocations that follow the redirect (probe-tree restore, base-tree rerun, discard-worktree sweeps) then execute the planted filter — host code execution out of the container. Round 13's live probe executed the planted smudge filter on the host (canary present; honest-gitfile control clean); no filter/fsmonitor/replace neutralisation has landed on the restore/creation paths since (only scratch-tree carries the identity gate). Until the gitfiles sit outside the RW mount (or the filter surface is otherwise neutralised), required cannot claim to contain host execution.
— qwen3.8-max via Qwen Code /review (v0.22.0)
`"Required"` in settings.json — or a stray trailing space — matched no policy,
resolved to `off`, and disabled the containment the operator had just asked
for. Silently. The environment value was already normalised; the settings
value was not, and the asymmetry fell on the wrong side: settings is the
documented place to turn this ON, since the environment can only tighten. So
the unnormalised half was the half operators actually use, and a fail-open on
the one setting whose whole purpose is to fail closed.
Reported as a suggestion in rounds 8, 9 and 16 and deferred each time under
the critical-only posture. Measured this round: both `"Required"` and
`"required "` returned `off`.
Also pins two shapes the new tests left open. `sandboxPolicy`'s settings
default is the production path every real caller takes, and with it swapped
for `{}` the whole settings half stopped being consulted while every
assertion — each passing settings explicitly — stayed green; it is now driven
through an isolated settings file. And the `SANDBOX_SET_UID_GID` opt-out
parses case- and space-insensitively, which nothing asserted.
The remaining deferral, a fixture literal pasted twice, is duplication rather
than a defect and is left alone.
commented
Aug 24, 2026
第 16 轮
归一化那条：我不再放它了，而且实测比你写的更宽它在第 8、9、16 轮都出现过，每次都因为「只处理 critical」被我延后。这轮我先量了： 不只是大小写，尾随空格一样中招。env 侧早就归一化了，settings 侧没有——而不对称落在了错的一边：settings.json 才是文档里推荐的开启方式（env 只能收紧）。也就是说，没做归一化的那一半，恰恰是操作者真正会用的那一半。操作者刚打开这个开关，就被静默关掉了，而这个开关存在的全部意义就是失败时要关向安全侧。 所以按建议级放了三轮之后，我把它当缺陷修了。变异：去掉归一化 → 红。 另外两条是我上一轮新测试自己留下的口子
没修的那条夹具字面量粘了两份——那是重复，不是缺陷，而这个 PR 已经太大。留着。 review 套件 100 文件 4446 通过 / 2 跳过。tsc / lint / prettier 干净。 R1-3状态不变：机制我不争，等的是维护者对范围的决定。 |
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- stale sandboxVerdict JSDoc (SANDBOX-set session 'returns direct') — already reported (comment 3836852398)
- SandboxVerdict.disclose produced but never surfaced in any phase output — already reported (comment 3836396804)
- orphaned restoreProbeTreeTracked doc comment after probeContainer insertion — already reported (comment 3836852318)
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): 324 passed — this review observed 23019 passed.
Deferred under the convergence posture (round 17, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:321 — [probe] normalization test pins only 'required' spellings — 'Auto'/'Off' operator spellings unpinned on both sides, mutant survivespackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:63 — [probe] garbled-env assertion pins only the empty-settings cell — the garbled-env + valid-settings interaction mutant survivespackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:345 — [probe] sandboxPolicy's env-half default (process.env) is exercised by no test — env={} default mutant survives both consuming suites
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
Test Plan（非阻断）：324 passed — this review observed 23019 passed。
收敛姿态下延后（第 17 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands at this head — deliberately deferred by the author since round 1 (reaffirmed in the round-10, round-14 and round-16 replies); the round-17 delta (the sandboxPolicy settings normalization plus tests) does not touch the mount geometry. The single RW bind mount covers the whole review temp dir (containerCommand mounts tmpDir:tmpDir RW at lines 622-623, read directly at the reviewed commit), so it covers every pipeline tree's .git gitfile: the gitfile sits inside its tree, i.e. inside this mount, and containerized PR code can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge; the host-side git invocations that follow the redirect (probe-tree restore, base-tree rerun, discard-worktree sweeps) then execute the planted filter — host code execution out of the container. Round 13's live probe executed the planted smudge filter on the host (canary present; honest-gitfile control clean); no filter/fsmonitor/replace neutralisation has landed on the restore/creation paths since (only scratch-tree carries the identity gate). Until the gitfiles sit outside the RW mount (or the filter surface is otherwise neutralised), required cannot claim to contain host execution.
中文说明
R1-3：在本 head 上依旧成立——自第 1 轮起由作者有意延后（第 10、14、16 轮答复中再次确认）；第 17 轮的增量（sandboxPolicy 的 settings 归一化及测试）没有触碰挂载几何。单一的 RW 绑定挂载覆盖整个 review 临时目录（containerCommand 在 622-623 行以 tmpDir:tmpDir RW 挂载，已在被审提交直接读取），因此也覆盖了每棵流水线树的 .git gitfile：gitfile 位于自己的树内、即位于本挂载之内，容器化的 PR 代码可以把它改写为携带 filter.<x>.smudge 的 gitdir: <同一挂载下的植入目录>；随后跟随该重定向的宿主侧 git 调用（探针树恢复、基线树重跑、丢弃工作树的清扫）就会执行植入的 filter——容器之外的宿主代码执行。第 13 轮的活体探针已在宿主上执行了植入的 smudge filter（金丝雀出现；诚实 gitfile 对照干净）；此后 restore/创建路径上没有落地任何 filter/fsmonitor/replace 中和（只有 scratch-tree 带身份门）。在 gitfile 移出 RW 挂载（或以其他方式中和 filter 面）之前，required 无法声称约束了宿主执行。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Last round's normalisation fix was asserted only against `required`
spellings, which is the shape a mutant keyed on that one value walks
straight through — leaving an operator's `"Auto"` resolving to `off`, the
same silent downgrade one rung lower. Assert every policy on both sides.
Two adjacent cells with it. A garbled ENVIRONMENT value must be dropped on
its own rather than taking the operator's setting down with it: the
environment is the half a repository can reach, so garbage there must never
answer for the half it cannot. And `sandboxPolicy`'s env default is the twin
of the settings default pinned last round — every assertion here hands it an
env literal, so `env = {}` as the default stops the environment half being
read at all and nothing notices.
Three mutants, three reds.
commented
Aug 24, 2026
第 17 轮
三条延后项都成立，都是我上一轮新测试的下一圈未钉单元，都修了：
三个变异，三次红。 说一句方法层面的观察从第 15 轮起，每轮的延后项都是「我上一轮新加的测试没有钉住的下一圈单元」。这个方向本身没有尽头——测试的测试的测试。我这三轮之所以还在跟，是因为每一圈里都还有真性质（仓库无法关掉容器化、env 垃圾不能替 settings 作答、生产默认路径无人经过）。等哪一轮的延后项只剩形状而没有性质了，我会停在那里并说明理由，而不是继续往下钉。 review 套件 99 文件 4444 通过 / 2 跳过。tsc / lint / prettier 干净。 R1-3状态不变。 |
left a comment
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- reviewSandboxImage's second pick (QWEN_CODE_CUSTOM_SANDBOX_IMAGE) untested in both directions — already reported (comment 3837175067)
- the allowlist canary inspects only the VALUES of the forwarded --env entries, never the SET — already reported (comment 3837175072)
Test Plan (not a blocker): 324 passed — this review observed 23019 passed.
Deferred under the convergence posture (round 18, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:422 — [probe] the containerPathFor test leaks its qwen-workdir-* temp fixture every run (the mountRootFor sibling cleans up in afterEach)packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:92 — [review] the paired SANDBOX_SET_UID_GID tests paste the containerCommand options literal twice instead of sharing one fixturepackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:730 — [review] killContainer — the reap itself — is executed by no test anywhere; garbling the spawnSync ships the suite greenpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:381 — [review] containerRuntime's real availability probe (docker-then-podman status check + cache) is exercised by no testpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:435 — [probe] containerPathFor's return-null branch is unpinned — probe-measured: dropping the try/catch ships green
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：324 passed — this review observed 23019 passed。
收敛姿态下延后（第 18 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // file one run rewrote, an ignored plant a sweep honoured. `--rm` is | ||
| // isolation by construction rather than by hygiene. | ||
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands at this head — deliberately deferred by the author since round 1; the round-18 delta is test-only and does not touch the mount geometry. The single RW bind mount covers the whole review temp dir (--volume ${tmpDir}:${tmpDir}, containerCommand lines 622-623), so it covers every pipeline tree's .git gitfile: the gitfile sits inside its tree, i.e. inside this mount. Containerized PR code can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge, and the host-side git invocations that follow the redirect (probe/base/scratch tree creation, post-run git operations) then execute the planted filter — host code execution. Witness: carried from round 1 (live probe — the gitfile redirect inside the mount reaches a planted repo and its filter executes on the host-side git call); re-verified at fd97d08 that the mount line is byte-identical. Fix direction: exclude each tree's .git gitfile from the RW mount (read-only bind or per-tree mounts), or have the host-side consumers refuse a gitdir redirect that stays under the mounted temp dir — the same fail-closed shape redirectedAncestor gives the mount walk.
中文说明
R1-3：在本 head 上依旧成立——自第 1 轮起由作者明确延后；第 18 轮的增量只动了测试文件，未触及挂载几何。单一 RW 绑定挂载覆盖整个 review 临时目录（--volume ${tmpDir}:${tmpDir}，containerCommand 第 622-623 行），因此也覆盖了每条流水线树的 .git gitfile：gitfile 位于树内，即位于此挂载之内。容器化的 PR 代码可以把它改写为 gitdir: <同一挂载下的植入目录>，其中带 filter.<x>.smudge；随后跟随该重定向的宿主侧 git 调用（探针/基线/草稿树的创建、运行后的 git 操作）就会执行植入的 filter——宿主代码执行。证据：沿用第 1 轮的活体探针（挂载内的 gitfile 重定向到达植入仓库，其 filter 在宿主侧 git 调用时执行）；并已在 fd97d08 复核挂载行逐字节未变。修复方向：把每棵树的 .git gitfile 排除在 RW 挂载之外（只读绑定或按树挂载），或让宿主侧消费者拒绝仍落在挂载临时目录之下的 gitdir 重定向——与 redirectedAncestor 为挂载游走提供的失败即关闭形态一致。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…ng a fixture Three of the four recorded this round; the fourth is duplication, not a defect, and is left alone for the third time. `killContainer` is the whole answer to a container that outlived the client's deadline, and nothing anywhere ran it — a garbled argv, or a dropped `-f`, shipped green, and what survives is a container holding the review tree open past the end of the run. It now takes its spawn as a parameter, so the argv and the swallow-and-continue contract are both asserted. `containerRuntime`'s probe had the same hole with a memo and a real daemon call in the way. The decision is now separated from both: `firstAnsweringRuntime` is order and nothing else, which is the content — a client installed but not running must never shadow one that is. And the `containerPathFor` case left a temp tree behind on every run. In a change about not leaving residue behind, that one is just embarrassing.
commented
Aug 24, 2026
第 18 轮
按我上一轮说的判据（钉的是性质还是形状）逐条筛，四条里做了三条：
第四条（夹具字面量粘两份）第三次出现，仍然不做：那是重复，不是缺陷。 review 套件 99 文件 4446 通过 / 2 跳过。tsc / lint / prettier 干净。 R1-3状态不变。 |
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): 324 passed — this review observed 23027 passed.
Deferred under the convergence posture (round 19, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/sandboxed-exec.ts:570 — [review] orphaned docblock: containerCommand's argv contract attached to containerSeqpackages/cli/src/commands/review/lib/sandboxed-exec.ts:151 — [probe] daemonAnswers answer-criterion and containerRuntime memo exercised by no testpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:451 — [probe] reap test's fake spawn discards the options bag — env scrub and timeout unpinnedpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:623 — [probe] containerCommand's runtime passthrough pinned only for docker, never podmanpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:690 — [probe] --tmpfs assertion pins only the path prefix; rw,mode=1777 unpinnedpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:210 — [probe] already-sandboxed anti-shortcut test misses the runtime-answering cellpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:911 — [probe] mountRootFor's catch→null branch untested; two gate chains have no catchpackages/cli/src/commands/review/lib/sandboxed-exec.ts:625 — [probe] --init flag pinned by no testpackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:920 — [probe] default image pinned only by substring 'sandbox'packages/cli/src/commands/review/lib/sandboxed-exec.test.ts:917 — [probe] pick().trim() normalization exercised by no fixturepackages/cli/src/commands/review/lib/sandboxed-exec.test.ts:566 — [probe] runtimeIsRootless default-reader wiring and info-document memo unpinned
[Critical] R19-1 (new this round, high confidence; dropped from inline only by the same-line overlap dedup against an unrelated round-1 comment at sandboxed-exec.ts:641, comment 3836852396 — the finding itself is verified, not a duplicate): The single RW bind mount (--volume ${tmpDir}:${tmpDir}, sandboxed-exec.ts:641-642) also covers the pipeline's worktree-LEASE files — leaseDirectory = join(repositoryRoot, REVIEW_TMP_DIR) in review-worktree-lease.ts, direct children of the mounted temp dir. The container runs as the host uid (--user uid:gid, or rootless root = host user), so reviewed code can read, plant, edit and delete leases. The session's own lease is written by fetch-pr before any containerized command runs and leaks sessionId/promptId/repositoryRoot. cleanupReviewWorktreeLeases matches leases by (sessionId, promptId) only — never validates provenance — then runs git worktree remove --force + git branch -D on whatever worktreePath/branch the lease names, invoked with the session's ids at prompt end/process exit (useGeminiStream.ts:4034, nonInteractiveCli.ts:546, Session.ts:5178). Failure scenario: during the networked npm ci (host uid, mount RW), a malicious postinstall reads sibling leases under <repo>/.qwen/tmp/ (every concurrent review of this checkout, enumerable from the mount), edits or plants a lease naming a victim session's ids with the worktreePath/branch of another session's tree — at the victim's prompt end, cleanup force-removes the wrong worktree and deletes the wrong branch: cross-session data loss from inside 'contained' execution. Cheaper wedge needing no secrets: plant a lease with any foreign sessionId → cleanup refuses the tree (cleanup.ts:746), fetch-pr throws on the EEXIST holder mismatch, and nothing ever sweeps a foreign-session lease — permanent denial of review for that PR on that machine until an operator deletes the file by hand. Witness (probe in a scratch checkout with two sessions Y/pr-1 and Z/pr-2, invoking the real cleanupReviewWorktreeLeases): edit-in-place of Y's lease (worktreePath → Z's tree): BEFORE review-pr-2 exists: true → AFTER review-pr-2 exists: false; planted sibling lease for pr-2 carrying Y's readable ids: AFTER review-pr-2 exists: false, AFTER branch qwen-review/pr-2 exists: false; foreign-sessionId wedge: blocks fetch-pr/cleanup of session "some-other-session": true, foreign lease survives the automatic sweep: true. Flip check: a lease-content guard flipped the edit-in-place arm but NOT the well-formed planted lease — content-indistinguishable, so the fix belongs at the mount boundary. Distinct from R1-3 (different asset, mechanism, harm). Fix direction: do not keep host-trusted lease state under the container's RW mount — move lease files out of REVIEW_TMP_DIR (e.g. a sibling dir like <repo>/.qwen/review-leases/), or validate lease provenance at read time before acting on it; whatever geometry fix lands for R1-3 should explicitly name this artifact class too.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：324 passed — this review observed 23027 passed。
收敛姿态下延后（第 19 轮，非阻断）——已记录，本轮不要求修改：共 11 条（原文未翻译，列表见上方英文部分）。
[Critical] R19-1 (new this round, high confidence; dropped from inline only by the same-line overlap dedup against an unrelated round-1 comment at sandboxed-exec.ts:641, comment 3836852396 — the finding itself is verified, not a duplicate): The single RW bind mount (--volume ${tmpDir}:${tmpDir}, sandboxed-exec.ts:641-642) also covers the pipeline's worktree-LEASE files — leaseDirectory = join(repositoryRoot, REVIEW_TMP_DIR) in review-worktree-lease.ts, direct children of the mounted temp dir. The container runs as the host uid (--user uid:gid, or rootless root = host user), so reviewed code can read, plant, edit and delete leases. The session's own lease is written by fetch-pr before any containerized command runs and leaks sessionId/promptId/repositoryRoot. cleanupReviewWorktreeLeases matches leases by (sessionId, promptId) only — never validates provenance — then runs git worktree remove --force + git branch -D on whatever worktreePath/branch the lease names, invoked with the session's ids at prompt end/process exit (useGeminiStream.ts:4034, nonInteractiveCli.ts:546, Session.ts:5178). Failure scenario: during the networked npm ci (host uid, mount RW), a malicious postinstall reads sibling leases under <repo>/.qwen/tmp/ (every concurrent review of this checkout, enumerable from the mount), edits or plants a lease naming a victim session's ids with the worktreePath/branch of another session's tree — at the victim's prompt end, cleanup force-removes the wrong worktree and deletes the wrong branch: cross-session data loss from inside 'contained' execution. Cheaper wedge needing no secrets: plant a lease with any foreign sessionId → cleanup refuses the tree (cleanup.ts:746), fetch-pr throws on the EEXIST holder mismatch, and nothing ever sweeps a foreign-session lease — permanent denial of review for that PR on that machine until an operator deletes the file by hand. Witness (probe in a scratch checkout with two sessions Y/pr-1 and Z/pr-2, invoking the real cleanupReviewWorktreeLeases): edit-in-place of Y's lease (worktreePath → Z's tree): BEFORE review-pr-2 exists: true → AFTER review-pr-2 exists: false; planted sibling lease for pr-2 carrying Y's readable ids: AFTER review-pr-2 exists: false, AFTER branch qwen-review/pr-2 exists: false; foreign-sessionId wedge: blocks fetch-pr/cleanup of session "some-other-session": true, foreign lease survives the automatic sweep: true. Flip check: a lease-content guard flipped the edit-in-place arm but NOT the well-formed planted lease — content-indistinguishable, so the fix belongs at the mount boundary. Distinct from R1-3 (different asset, mechanism, harm). Fix direction: do not keep host-trusted lease state under the container's RW mount — move lease files out of REVIEW_TMP_DIR (e.g. a sibling dir like <repo>/.qwen/review-leases/), or validate lease provenance at read time before acting on it; whatever geometry fix lands for R1-3 should explicitly name this artifact class too.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| '--volume', | ||
| `${opts.tmpDir}:${opts.tmpDir}`, |
There was a problem hiding this comment.
[Critical] R1-3: still stands at this head — deliberately deferred by the author since round 1 (reaffirmed in the round-10, round-14 and round-16 replies); the round-19 delta (the firstAnsweringRuntime extraction, killContainer's injectable spawn parameter, and their tests) does not touch the mount geometry. The single RW bind mount covers the whole review temp dir (--volume ${tmpDir}:${tmpDir}, containerCommand lines 641-642), so it covers every pipeline tree's .git gitfile: the gitfile sits inside its tree, i.e. inside this mount. Containerized PR code can rewrite it to gitdir: <planted dir under the same mount> carrying filter.<x>.smudge, and the host-side git invocations that follow the redirect (probe/base/scratch tree creation, post-run git operations) then execute the planted filter — host code execution. Witness: carried from round 1 (live probe — the gitfile redirect inside the mount reaches a planted repo and its filter executes on the host-side git call); re-verified at 62040e4 that the mount line is byte-identical through this round's delta. Fix direction: exclude each tree's .git gitfile from the RW mount (read-only bind or per-tree mounts), or have the host-side consumers refuse a gitdir redirect that stays under the mounted temp dir — the same fail-closed shape redirectedAncestor gives the mount walk. Note: round 19 surfaced a sibling artifact of the same mount width — worktree-lease files — reported in the review body.
中文说明
R1-3：在本 head 上依旧成立——自第 1 轮起由作者明确延后（第 10、14、16 轮的回复中再次确认）；第 19 轮的增量（firstAnsweringRuntime 提取、killContainer 的可注入 spawn 参数及其测试）未触及挂载几何。单一 RW 绑定挂载覆盖整个 review 临时目录（--volume ${tmpDir}:${tmpDir}，containerCommand 第 641-642 行），因此也覆盖了每条流水线树的 .git gitfile：gitfile 位于树内，即位于此挂载之内。容器化的 PR 代码可以把它改写为 gitdir: <同一挂载下的植入目录>，其中带 filter.<x>.smudge；随后跟随该重定向的宿主侧 git 调用（探针/基线/草稿树的创建、运行后的 git 操作）就会执行植入的 filter——宿主代码执行。证据：沿用第 1 轮的活体探针（挂载内的 gitfile 重定向到达植入仓库，其 filter 在宿主侧 git 调用时执行）；并已在 62040e4 复核挂载行在本轮增量中逐字节未变。修复方向：把每棵树的 .git gitfile 排除在 RW 挂载之外（只读绑定或按树挂载），或让宿主侧消费者拒绝仍落在挂载临时目录之下的 gitdir 重定向——与 redirectedAncestor 为挂载游走提供的失败即关闭形态一致。注：第 19 轮发现了同一挂载宽度下的另一类产物——worktree 租约文件——已在评审正文中报告。
— qwen3.8-max via Qwen Code /review (v0.22.0)
commented
Aug 24, 2026
第 19 轮R19-1 我逐条核过，成立——而且有一处你的表述需要更正，方向对你不利也对我不利，先说清楚。 核实的部分三条关键事实都对上了：
它与 R1-3 确实是不同的资产、机制和危害，但同一个根因：宿主可信状态待在容器的可写面之内。 需要更正的部分
这个措辞会让人以为是容器化制造了这个暴露面。不是。我去 也就是说，今天在 main 上，被审仓库的 所以 R19-1 是既有漏洞，不是本 PR 引入的回归。这不是推脱：它仍然是真问题，仍然该修；但归因错了会让人以为「不上容器更安全」，而事实恰好相反。 我做了什么，没做什么做了：把这个资产类别写进 PR 描述的「本 PR 关不掉的」一节，和 gitfile 并列点名，并写明两者都不是本改动打开的、以及为什么。一个声称容器化的 没做：没有在本 PR 里改 lease 的落盘位置。理由有三条，第三条是你自己给的：
所以它和 R1-3 一起，作为同一个待定范围决策的一部分交给维护者。本轮其余 11 条延后项按同一判据（钉的是性质还是形状）留待下一轮。 |
# Conflicts: # packages/cli/src/commands/review/lib/review-settings.ts
commented
Aug 25, 2026
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 83 passed · 0 failed · 83 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：83 通过 · 0 失败 · 83 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9723 deep verification — review sandbox (
|
| cell | code under test | env canary visible | <repo>/.git readable |
executions |
|---|---|---|---|---|
| base-direct | HEAD^1 spawn pattern | yes (predicted) | yes | 1 |
| head-off | head run(), policy off |
yes (= base) | yes | 1 |
| head-auto, no runtime | head run(), auto |
yes (direct fallback, by design) | yes | 1 |
| head-required, no runtime | real refuseUnsandboxedPhase + full runTestDelta chain |
n/a | n/a | 0 — "NOTHING was attributed" |
| head, no policy (delta) | real runTestDelta |
n/a | n/a | 1 (no blanket refusal) |
17/17 assertions. The base cell's leak is the control assertion (base is
expected to leak — that is the defect the PR exists to close), encoded as a
passing expectation in the harness.
The author's "checkable rather than argued" base claim was checked by direct
reading of the parent commit and holds: build-test.ts passed
env: buildRunEnv() (full spread) and test-efficacy.ts's probe spawn passed
no env key at all (full inheritance).
Contained cells against the live daemon (03-contained-test-run.png)
This verify container ships no docker client but a live daemon on
/var/run/docker.sock (engine 29.1.3 — the same lineage the PR's measurements
cite). I supplied a static docker client 29.7.2 outside the repo tree. One
environmental limit, proven by its own cell: bind mounts of this container's
filesystem do not propagate to the daemon's containers (the daemon does not
share its fs namespace), so contained commands were self-contained
(env/id/HOME/network probes) rather than host-fixture-backed. Everything that
does not need shared files was verified against real containers created by the
module's own argv (run() → containerised() → containerCommand()), with
QWEN_REVIEW_SANDBOX_IMAGE=node:22-bookworm (cached on the daemon) via the
documented override:
| property | test kind | install kind |
|---|---|---|
| exit 0, ran inside container | ✓ | ✓ |
canary OPENAI_API_KEY/GH_TOKEN absent |
✓ | ✓ |
| env is exactly the allowlist (CI, npm_config_yes, QWEN_SKIP_PREPARE, HOME=tmpfs, cache under mount; image defaults PATH/HOSTNAME only) | ✓ | ✓ |
id = uid 1000 gid 1000 (host uid mapping) |
✓ | ✓ |
$HOME is tmpfs and writable by the mapped uid |
✓ | ✓ |
| loopback fixture server works | ✓ | ✓ |
| egress to registry.npmjs.org | blocked (EAI_AGAIN) | ok:200 |
runtimeIsRootless('docker') against the live info document: 8 991-byte
JSON, no rootless marker → rootful → --user correctly retained (23/23
assertions across the five runtime cells, including the reap cell below).
Reap-by-name on deadline (real daemon, 03-reap.log)
Workload trap "" TERM; sleep 60, 3 s deadline through the real run():
result {exitCode: null, timedOut: true} (client SIGKILLed at the deadline);
the container was polled Up mid-run (11 sightings across 3 state lines) and
docker ps -a --filter name=qwen-review- was empty after run() returned —
killContainer reaped it by name. 3/3.
Corrections
None — no prior review round; I also found no inaccurate statement in the PR
text that survived contact with the code (the three headline decisions, the
SANDBOX-is-not-a-shortcut rule, the hand-off conversion at the single exit,
and the base-side leak all reproduce as described).
Findings
S1 (Suggestion, non-blocking): the phase-gate invocations in build-test and test-efficacy are not pinned by any test — mutation M1 survives
Deleting the entire gate block from runBuildTestUnguarded
(const refusal = refuseUnsandboxedPhase(root); … if (refusal) return refusedReport(refusal);) leaves build-test.test.ts at 109 passed (109).
The twin one-liner in runTestEfficacy
(refuseUnsandboxedPhase(probeWorktreePath(worktree))) is in the same class:
the integration test's new lines only isolate the operator's settings so other
tests don't inherit a required, nothing drives the gate. Classification per
the survivor taxonomy: ordinary coverage gap — not dead code (harness 01
proves the call is load-bearing: run() itself does not enforce required,
so without the gate the same scenario executes the reviewed commands
directly), not redundant defence (no other hunk covers these routes). The gate
function is thoroughly pinned in sandboxed-exec.test.ts, and test-delta's
wiring is pinned by its own test ("refuses the base-side rerun under
required"); only the build-test and test-efficacy call sites are unpinned.
The author says exactly this for build-test in the round-3 commit message
("What stays unpinned is only that runBuildTest calls it, which is one
visible line"). The fixture that would go red: with
QWEN_REVIEW_SANDBOX=required and no runtime on PATH,
runBuildTest({root: <npm fixture>, …}) must return toolchain: 'refused',
ok: false, with its injected exec seam invoked zero times — the mirror
of the test test-delta.test.ts already has for its own gate.
No other finding survived the round. Specifically probed and not findings:
- Sibling sweep of execution sites: every place the pipeline executes the
reviewed repo's commands routes through the gate —base-treebuilds via
runBuildTest;test-delta's rerun was re-pointed at build-test's single
run()(the duplicate that could drift is gone);script-lintspawns only
host-installed linters over text (a different hazard class with its own
config isolation). No ungated route found. - Network per kind is structural: exactly one
execcall passes
'install'; every other call (build, suite, retries, base-side rerun) takes
the default, which is the restrictive'test'— a future adapter cannot
silently grant egress. - The second-continuation-exit invariant (refusal on
--resumethrows
instead of overwriting the in-flight report) is pinned by
resumeWouldDestroyReporttests plus the gate's own throw.
Targeted gates
packages/clireview suite (npx vitest run src/commands/review):
5 017 passed | 5 skipped | 0 failed (logs/review-suite-rerun.log),
plus per-file runs ofbuild-test.test.ts(109/109) and
sandboxed-exec.test.ts(35 passed | 1 skipped) during the mutation round.
The PR body cites 4 324 passed / 1 skipped — a count from an earlier stage
of the branch; the final head's later test commits grew the suite. Zero
failures on both sides is the comparison that matters.- Mutation matrix (vacuity): control green (35 passed | 1 skipped on the
unmutatedsandboxed-exec.test.ts); M0/M2/M3/M4 killed with the intended
assertion messages quoted inlogs/05-detail.log
(e.g. M3:expected 'off' to be 'required'); M1 survived as classified
above.04-mutation-matrix.png. - Generated artifact:
npm run generate:settings-schemareproduces the
committedpackages/vscode-ide-companion/schemas/settings.schema.json
byte-for-byte (git diffempty after regeneration).
Not covered
- Per-commit attribution — checkout is depth 2 (merge commit + parents
only);git rev-list HEAD^1..HEAD^2shows 1 commit locally vs 23 in the
metadata snapshot, i.e. the shallow boundary. The aggregate
HEAD^1..HEADdiff is what was verified; the 23-commit history (four
fix rounds) was not individually exercised. - Host-file-backed contained runs — bind mounts do not propagate from this
container to the daemon (proven by themount-propagationcell: a marker
file mounted with the module's exact-vspelling is ENOENT inside). So
"the container resolves the dependency farm's links through the mount" and
"a contained suite produces the same verdicts" remain untested end-to-end
here — the same gap the author names as not done. The mount string, the
deepest-root arithmetic, and the symlink/colon refusals are verified
against the real filesystem in harness 02. - Native module builds inside the image (
packages/audio-capture/node-gyp
class), rootless podman (no podman here), Windows cells
(win32-only tests skip on Linux), realnpm ciregistry load under the
install-kind network. - CI wiring (
QWEN_REVIEW_SANDBOX=requiredin workflows) — deliberately a
separate change per the PR description. - Host-trusted state under the mount (gitfile redirects, worktree lease
files): the PR discloses these itself as pre-existing and not widened
(consistent with base: onmainthe same commands run with no filesystem
restriction at all). Not re-litigated here. - The
verify-capture.mjscaptures for the reap cell and the policy attack
cell were not separately imaged; their raw logs are inlogs/03-reap.log
andlogs/02-argv-oracle.log.
Methodology
Environment: CI verify container (node:22-bookworm, uid 1000, no docker
client but a reachable daemon at /var/run/docker.sock, engine 29.1.3; static
docker client 29.7.2 fetched to /__w/_temp/docker-cli, outside the repo).
npm ci + npm run build had completed at the merge head before this round;
all head-side harnesses import the compiled packages/cli/dist output, so the
verified artifact is the shipped JS, not a re-translation. Harnesses
(harness/01-ab-leak.mjs, 02-argv-oracle.mjs, 03-real-runtime.mjs,
04-mutations.sh, m1-mutate.mjs) run each cell in a fresh child process
(sandboxed-exec memoises its runtime probe at module scope; loadEnvironment
mutates process.env), and every assertion is a scripted comparison that can
fail — counts above come from those runs only. Mutations were applied in place
and restored with git checkout, each restore verified by a clean
git status and the four hunk greps; the v1 mutation script had a broken
restore path which accumulated mutants, was caught by its own red control, and
redone (v2 logs are authoritative). Raw per-cell output lives in logs/
(01-ab-leak.log, 02-argv-oracle.log, 03-real-runtime.log, 03-reap.log,
m0–m4, m-ctrl.txt, m1.txt, 05-detail.log).
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/cli/src/commands/review/build-test.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/build-test.test.ts
file packages/cli/src/commands/review/lib/sandboxed-exec.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/sandboxed-exec.test.ts
file packages/cli/src/commands/review/test-delta.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/test-delta.test.ts
file packages/cli/src/commands/review/test-efficacy.integration.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/test-efficacy.integration.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/build-test.test.ts: PPPPP
packages/cli/src/commands/review/lib/sandboxed-exec.test.ts: PPPPP
packages/cli/src/commands/review/test-delta.test.ts: PPPPP
packages/cli/src/commands/review/test-efficacy.integration.test.ts: PPPPP
verdict: pass
summary: 4 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/build-test.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/sandboxed-exec.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/test-delta.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/test-efficacy.integration.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/build-test.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/sandboxed-exec.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/test-delta.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/test-efficacy.integration.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/build-test.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/sandboxed-exec.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/test-delta.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/test-efficacy.integration.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/build-test.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/sandboxed-exec.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/test-delta.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/test-efficacy.integration.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/build-test.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/sandboxed-exec.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/test-delta.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/test-efficacy.integration.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
commented
Aug 25, 2026
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
commented
Aug 26, 2026
|
Released in v0.22.2. |
5 participants
What this PR does
A review executes the code it is reviewing. This puts that execution behind a container boundary, and makes it a policy the operator sets rather than a property of what happens to be installed.
Two call sites run the reviewed repository's own commands:
build-test.tspackage.jsonnames —npm ciwith itspreinstall/postinstall, the build, the suitetest-efficacy.tsBoth now route through
lib/sandboxed-exec.ts, which either returns a container argv ornullfor the direct spawn that has always been there.Why it's needed
Both call sites handed the PR's code
process.enventire —buildRunEnv()spreads it, and the probe spawn passes noenvkey at all so it inherits. On CI that environment carriesOPENAI_API_KEYandGH_TOKEN. Apostinstallscript readingprocess.envis one line, and it needs none of the git-config machinery the pipeline's twenty rounds of threat findings were built on.That is checkable rather than argued: read the two call sites at the parent commit.
The boundary goes around the executions, not around the review agent. Wrapping the agent was tried first and is the wrong shape — its secrets do not survive the container's env allowlist (
sandbox.tsforwards by name, andOPENAI_*/GH_TOKENare not on the list), itstimeoutreaps the host-side docker client rather than the container, its CLI version stops matching the runner's@latest— and after paying all of that the mount isprocess.cwd(), the whole checkout, so<repo>/.gitstays writable anyway.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review→ 4 324 passed, 1 skipped, 0 failed. The default policy isoff, so every existing test exercises the unchanged direct path; the new file pins the argv.Evidence (Before & After)
Three decisions carry this design, each measured rather than assumed, each pinned by a test that goes red when that decision alone is reverted:
node_modulespoints at the review worktree's copy — 1 722 links, 0 failed on a live CI review (run 32423107998). Mounting one tree leaves every link dangling--network nonekeeps loopback, so a suite standing up a local fixture server still runsOne ephemeral container per command, not one per phase. Measured on the ECS pool from a real sandboxed autofix job (96386571484):
Resolve sandbox imageis 2 s once, warm; adocker runon a warm image adds a few hundred ms. AgainstTOTAL_BUDGET_MS = 540_000and at most ~15 suite runs that is 1–2 %. The stronger reason is not cost: a long-lived container per phase would re-introduce the cross-run state #9221 spent rounds closing (tracked-file carryover, ignored-plant carryover).--rmis isolation by construction.Tested on
macOS 26.6 (Darwin 25.6.0), Node 24,
packages/clivitest 3.2.4. No container runtime on this machine — which is why the argv is what the tests pin, and why an integration exercise against a real runtime is named below as not done.Risk & Scope
Off by default.
review.sandboxisoff|auto|required;offis today's behaviour exactly. Containerising a build by surprise changes what native modules compile against, so nobody gets it without asking.Read through
operatorReviewSettings, which skips the workspace scope — a repository cannot ship a.qwen/settings.jsonthat switches off the containment that exists to contain it.QWEN_REVIEW_SANDBOXoutranks the setting so CI can require containment without depending on a settings file the runner may not carry.requiredmakes the execution-dependent evidence unavailable for that run; it does not end the review. That is the rule this repository already established one level down — a probe that cannot get an isolated tree reportsinconclusiverather than falling back to the shared worktree.Not done in this PR, and I would rather name it than let it read as done:
npm ciinside the image must produce the same native deps the host does; this repo already trips onpackages/audio-capture/node-gyp.QWEN_REVIEW_SANDBOX=requiredand copies autofix'sdocker infopreflight is deliberately a separate change, so this one can land and be exercised withautofirst..gitgitfile (rewrite it and the host-side git that follows the redirect runs a planted filter), and the worktree lease files, whichcleanupReviewWorktreeLeasesmatches by session ids alone before force-removing whatever worktree and branch they name. Neither is opened by this change — onmainthe reviewed repository's commands already run as the host user with no filesystem restriction at all, so both are reachable there and reachable more widely; containment narrows what such a script can touch rather than widening it. But it does not close them, and arequiredthat claims containment should say so rather than imply otherwise. Closing them is a mount-geometry change (keep host-trusted state out of the writable surface), tracked as its own work.Linked Issues
Implements the decision on #9556, with the design, the measurements and the correction to my own earlier framing recorded in that thread.
中文说明
这个 PR 做了什么
审查会执行它所审查的代码。本 PR 把那次执行放到容器边界之后，并让它成为操作者设定的策略，而不是"恰好装了什么"的副产品。
两个调用点在运行被审仓库自己的命令：
build-test.ts（被审仓库package.json指定的一切——npm ci连同preinstall/postinstall、构建、测试套件）与test-efficacy.ts（该套件再跑很多遍：基线／对照／每个突变体／每个 hunk 探针／回退）。两者现在都经由lib/sandboxed-exec.ts，它要么返回容器 argv，要么返回null走一直以来的直接 spawn。为什么需要
两个调用点都把
process.env整个交给了 PR 的代码——buildRunEnv()展开它，而探针 spawn 干脆没有env键因而继承。在 CI 上那里面有OPENAI_API_KEY与GH_TOKEN。一个读process.env的postinstall就是一行，且完全不需要二十轮威胁 finding 所依赖的那套 git 配置机关。这可核查而非可辩论：读父提交上的那两个调用点即可。
边界围住的是执行，而不是审查 agent。 先试的是包住 agent，形状不对——它的密钥过不了容器的 env 白名单（
sandbox.ts按名字转发，OPENAI_*/GH_TOKEN不在名单上）、它的timeout收割的是宿主侧 docker 客户端而非容器、它的 CLI 版本与 runner 的@latest不再一致；而付完这些代价之后，挂载仍是process.cwd()（整个检出目录），<repo>/.git照样可写。审查者验证方案
如何验证
cd packages/cli && npx vitest run src/commands/review→ 4 324 通过、1 跳过、0 失败。默认策略是off，因此所有既有测试跑的都是未改变的直接路径；新文件钉的是 argv。证据（Before & After）
三条决定支撑本设计，每条都经过测量而非假设，且每条都有测试钉住（单独回退该决定即变红）：
node_modules里每个包都指向 review 工作树的副本——真实 CI 审查实测 1 722 条链接、0 失败（run 32423107998）。只挂一棵树会让每条链接悬空--network none保留回环，因此"起本地夹具服务器"的套件照常工作每命令一个一次性容器，而非每阶段一个。在 ECS 池上、从一次真实沙箱化 autofix 作业（96386571484）实测：
Resolve sandbox image预热下 2 秒、每作业一次；对预热镜像docker run额外数百毫秒。对上TOTAL_BUDGET_MS = 540_000与至多约 15 次套件运行，占 1–2%。更强的理由不是成本：每阶段长驻容器会把 #9221 花了若干轮才关掉的跨运行状态重新引进来（已跟踪文件残留、被忽略投毒残留）。--rm是构造上的隔离。测试环境
macOS 26.6（Darwin 25.6.0）、Node 24、
packages/clivitest 3.2.4。本机没有容器运行时——这正是"测试钉的是 argv"的原因，也是下面把"针对真实运行时的集成验证"明确列为未做的原因。风险与范围
默认关闭。
review.sandbox取off|auto|required；off与今天的行为完全一致。让构建在无人要求的情况下进容器，会改变原生模块编译所依赖的对象。它经由
operatorReviewSettings读取，而该函数跳过 workspace 作用域——仓库无法通过.qwen/settings.json关掉那道正是为了约束它自己而存在的containment。QWEN_REVIEW_SANDBOX优先级高于该设置，使 CI 无需依赖 runner 上未必存在的设置文件即可强制要求。required会让依赖执行的证据在该次运行中不可用，但不会终止审查。这正是本仓库低一层已经立好的规矩——拿不到隔离树的探针报inconclusive，而不是退回共享工作树。本 PR 未做、且我宁愿点名也不愿让人读成已做的：
npm ci必须产出与宿主相同的原生依赖；本仓库已在packages/audio-capture/node-gyp 上踩过。QWEN_REVIEW_SANDBOX=required并照抄 autofixdocker info前置的那次工作流改动，有意留作另一个 PR，以便本 PR 先落地并用auto实际磨合。.gitgitfile（改写它，跟随重定向的宿主侧 git 就会执行植入的 filter），以及工作树 lease 文件——cleanupReviewWorktreeLeases仅凭会话标识匹配，随后对 lease 里写的工作树和分支执行强制删除。这两条都不是本改动打开的：在main上，被审仓库的命令本来就以宿主用户身份、在毫无文件系统限制的情况下运行，因此那里同样够得到、而且够得更宽；容器化是把这类脚本能碰到的范围收窄，不是扩大。但它确实没有关掉这两条，而一个声称容器化的required应当把这件事说出来，而不是让人以为相反。关掉它们属于挂载几何的改动（把宿主可信状态移出可写面），另行跟踪。关联 Issue
实现 #9556 上的决策；设计、测量，以及我对自己先前表述的更正，都记录在该讨论串中。