fix(review): keep host-trusted state out of the container's writable surface - #9983
fix(review): keep host-trusted state out of the container's writable surface#9983wenshao wants to merge 15 commits into
Conversation
…surface Two instances of one root cause, both reachable on `main` today and neither opened by the sandbox — before it, the reviewed repository's commands ran as the host user with no filesystem restriction at all, so containment narrowed this reach rather than creating it. What it did not do is close it, and a `required` that claims containment should not leave host-trusted state in the directory it hands the container read-write. **Leases.** `cleanupReviewWorktreeLeases` matches a lease by session ids alone and then force-removes whatever worktree and deletes whatever branch it names. Those files lived in the review temp dir — the bind mount. Reviewed code that edits one makes another session's cleanup destroy the wrong tree; one it plants with a foreign session id is swept by nobody and wedges that PR on that machine until someone deletes the file by hand. They now live one directory over, in `.qwen/review-leases`, which nothing mounts. A lease left behind by an older build is deleted when the same target is leased again. **Gitfiles.** These cannot move — git requires `<tree>/.git` — so the check moves instead. The existing identity gates cannot see the attack and never could: the writer chooses both the gitfile's target and that target's backpointer, so the round-trip agrees; `--show-toplevel` still names the tree; no symlink is involved; and asking git for the common dir resolves THROUGH the rewritten gitfile, so that answer comes from the same hand. Location is the question that has an honest answer — a real admin entry lives under `<repo>/.git/worktrees/`, outside the mount, and a planted one has to be inside it. `restoreProbeTreeTracked` now refuses an entry that resolves inside the review temp dir. Reproduced end to end before fixing, and the reproduction is the reason the gate is where it is: a rewritten gitfile pointing at a planted admin entry whose own `commondir` names a planted repository, both under the mount, passes all three existing gates — and `git checkout --force` then runs that repository's `filter.evil.smudge` ON THE HOST. With the new gate the same shape is refused while the entry it replaced still passes. Three mutants, three reds: a gate that always admits, one that admits when the entry cannot be resolved, and one that refuses when there is no mount at all — the last would refuse every ordinary repository.
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. |
|
Thanks for the PR! Template looks good ✓ Problem: demonstrated, not theoretical. R19-1 and R1-3 were disclosed as not closed in #9723's review, and the description carries a build-it-by-hand reproduction in which a planted admin entry passes all three existing identity gates and executes a smudge filter on the host during Direction: aligned. Host-trusted state does not belong inside the directory the review container mounts read-write — that is the containment contract the pipeline claims — and both findings are the same root cause, so one fix is the right call. No user-facing surface changes. Size: touches Approach: minimal and coherent — move the leases one directory over (out of the mount, lifecycle unchanged), and add a location gate where the identity gates structurally have no honest answer. No drive-by changes in the diff. Risk: no high-risk-path matches; no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已被证实，不是理论性的。R19-1 与 R1-3 在 #9723 的评审中已被披露为「关不掉」，且描述里给出了可手工构建的复现：植入的管理条目通过全部三道现有身份门，并在 方向：对齐。宿主可信状态不应留在 review 容器以读写挂载的目录之内——这是流水线声称的容器化契约——而两条发现同根同源，一次修复是正确的选择。无用户可见面变更。 规模：触及 方案：最小且自洽——把 lease 移到隔壁目录（移出挂载面，生命周期不变），并在身份门结构上没有诚实答案的地方加一道位置门。diff 中没有顺手改动。 风险：未命中高风险路径；无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe approach matches what I'd have proposed from the description alone — move the trusted state out of the writable surface, and answer the gitfile attack with location instead of identity, since identity is chosen by the attacker on both ends of the round trip. What I verified while reading it:
One real problem, though. Critical — the gate admits an admin entry that resolves to the mount root itself. Not blocking, but worth the record:
Testing evidence (this PR's own CI — the review is static, PR code is never executed here)The ubuntu unit suite is still in flight at review time; the finalize pass updates the table once it lands. Security checks are green, and the macOS/Windows test jobs are skipped by workflow design for this PR. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The unit suite pins the predicate and the lease relocation, but the end-to-end claim — that a host 中文说明代码评审 方案与我仅凭描述独立提出的思路一致——把可信状态移出可写面，用「位置」而非「身份」回答 gitfile 攻击，因为往返的两端都是攻击者选的。阅读中核实到的：
但有一个真问题。 关键问题——这道门会放行「恰好解析到挂载根本身」的管理条目。 不阻塞，但留档：
测试证据（本 PR 自己的 CI——评审是静态的，这里从不执行 PR 代码） 评审时 ubuntu 单元测试仍在运行，finalize 会在其落定后更新表格。安全检查为绿，macOS/Windows 测试作业按工作流设计对此 PR 跳过。单元测试钉住了谓词与 lease 迁移，但端到端论断——宿主 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — everything about this PR is right except one boundary term in the gate itself, and that boundary is the whole point of the PR. Stepping back: the problem is real and demonstrated, the root-cause unification (R19-1 + R1-3 = host-trusted state inside the writable surface) is the correct read, the lease move is clean and provably complete, and the location-over-identity idea is exactly right — identity questions have no honest answer here, location does. My independent proposal from the description alone landed on the same two moves, which is a good sign for the design. Where it diverges is the containment test. I would have written "inside" as Requesting changes on that one finding; with it fixed (and a green unit suite on the CI table above) this is ready. 🙏 中文说明置信度：2/5 —— 这个 PR 的一切都是对的，除了门本身的一个边界项，而那个边界正是本 PR 的全部意义所在。 退一步看：问题是真实且已被证明的；根因归并（R19-1 + R1-3 = 宿主可信状态位于可写面之内）是正确的判断；lease 迁移干净且可证明地完整；「用位置代替身份」的思路完全正确——身份问题在这里没有诚实答案，位置有。我仅凭描述独立提出的方案也落在同样两步上，这对设计是个好信号。 分歧在容器判定本身。我会把「在里面」写成 就这一条发现请求修改；修复之后（加上面 CI 表格中变绿的单元测试）即可合并。🙏 — 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.
One blocking finding: the new location gate admits an admin entry that resolves to the mount root itself — the boundary term re-opens the exact attack class this PR closes. Details and the one-line fix are in my review note above. 🙏
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: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 23879 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 23879 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| return true; | ||
| } | ||
| const rel = relative(realRoot, real); | ||
| return rel !== '' && !rel.startsWith('..') && !isAbsolute(rel); |
There was a problem hiding this comment.
[Critical] R1-1: The new containment predicate fails open for two shapes. An admin entry whose realpath is the mount root itself: relative(realRoot, real) returns '', and the rel !== '' conjunct classifies it as "not inside" — but the mount root is as writable by reviewed code as anything under it. And a direct child of the mount root whose name starts with .. (a legal filename, e.g. ..evil-git): relative() returns '..evil-git', and startsWith('..') misclassifies it as outside. Reviewed code in the sandbox rewrites the probe tree's .git gitfile to gitdir: <repo>/.qwen/tmp (or <repo>/.qwen/tmp/..evil-git) and plants a config with filter.evil.smudge plus a matching backpointer there; every pre-existing gate passes because the attacker writes both ends of the round-trip, and the host-side git checkout --force HEAD -- . then executes the planted smudge filter outside the sandbox — the exact escape this PR exists to close, left open at two boundary points. The hand-rolled check duplicates core's existing isSubpath (packages/core/src/utils/paths.ts), which handles the .. case correctly. A real pipeline entry lives under <repo>/.git/worktrees/ — never the mount root, never a weirdly-named child of it — so counting both shapes as inside has no false-positive cost.
Witness (probe driving the real function; flip check passes):
PROBE FAIL-OPEN shape(a) entry==mount-root: observed=false expected(safe)=true
PROBE FAIL-OPEN shape(b) child ..evil-git: observed=false expected(safe)=true
after the fix below: PROBE-RESULT: all safe (both shapes refused, controls unchanged)
| return rel !== '' && !rel.startsWith('..') && !isAbsolute(rel); | |
| return rel === '' || (rel !== '..' && !rel.startsWith(`..${sep}`) && !isAbsolute(rel)); |
Also add sep to the node:path import above (or reuse isSubpath), and add both shapes to the adminEntryInsideReviewTmp test block.
中文说明
新的包含判断对两种形态失守（fail-open）。其一，管理条目的 realpath 恰好是挂载根本身：relative(realRoot, real) 返回 ''，而 rel !== '' 这一条件把它判为「不在其内」——但挂载根本身与被审代码可写的任何子目录一样可写。其二，挂载根下以 .. 开头的直接子目录（合法文件名，如 ..evil-git）：relative() 返回 '..evil-git'，startsWith('..') 把它误判为「在外」。沙箱内的被审代码可以把探针树的 .git gitfile 改写为 gitdir: <repo>/.qwen/tmp（或 <repo>/.qwen/tmp/..evil-git），并在那里植入带 filter.evil.smudge 的 config 和自洽的 backpointer；由于往返两端都由攻击者写入，已有的所有门都会通过，随后宿主侧的 git checkout --force HEAD -- . 就会在沙箱外执行植入的 smudge 过滤器——这正是本 PR 要关闭的逃逸，却在两个边界点上仍然敞开。这段手写判断其实重复了 core 里现成的 isSubpath（packages/core/src/utils/paths.ts），后者对 .. 的处理是正确的。真实的流水线管理条目位于 <repo>/.git/worktrees/ 之下——既不会是挂载根，也不会是挂载根下名字怪异的子目录——因此把这两种形态判为「在内」没有任何误报成本。
证据（对真实函数的探针；翻转检查通过）：
PROBE FAIL-OPEN shape(a) entry==mount-root: observed=false expected(safe)=true
PROBE FAIL-OPEN shape(b) child ..evil-git: observed=false expected(safe)=true
应用下方修复后: PROBE-RESULT: all safe（两种形态都被拒绝，对照组不变）
修复见上方 suggestion 代码块（同时把 sep 加入上方的 node:path 导入，或改用 isSubpath），并在 adminEntryInsideReviewTmp 测试块中补上这两种形态。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // the way — and the `checkout --force` below then runs whatever | ||
| // `filter.<x>.smudge` that directory declares, on the HOST. See | ||
| // `adminEntryInsideReviewTmp`. | ||
| if (adminEntryInsideReviewTmp(gitDir, mountRootFor, probeTree)) { |
There was a problem hiding this comment.
[Critical] R1-2: Every identity gate in restoreProbeTreeTracked — the backpointer round-trip, this new location gate, redirectedAncestor — sits inside the if (lstatSync(join(probeTree, '.git')).isFile()) branch. Reviewed code that replaces the gitfile with a .git directory under the mount (e.g. rm .git && git init . && git config filter.evil.smudge CMD, all inside the RW mount) makes isFile() false, skips the whole gate block, and the host-side checkout --force / clean -ffdx then run against the attacker's repository. The pipeline never creates that shape under the mount — probe trees are always git worktree add (a gitfile) — so refusing it has no false-positive cost; the diff's "a plain checkout has a .git directory" rationale argues the unmounted state, where mountRootFor returns null and this guard would not fire.
Witness (probe with real git through the exact restore spawns):
--- before attack: .git is a gitfile
--- marker file created by smudge: YES
(The -c core.hooksPath=/dev/null/no-hooks -c core.fsmonitor= flags neutralize hooks and fsmonitor only — not filters.)
Suggested fix, before the isFile() branch:
if (mountRootFor(probeTree) !== null && !lstatSync(join(probeTree, '.git')).isFile()) {
return `${probeTree}'s .git is not the pipeline's gitfile — refusing a shape the pipeline never creates under the mount`;
}中文说明
restoreProbeTreeTracked 里的所有身份门——backpointer 往返、这道新的位置门、redirectedAncestor——都位于 if (lstatSync(join(probeTree, '.git')).isFile()) 分支之内。被审代码只要把 gitfile 换成挂载之内的一个 .git 目录（例如 rm .git && git init . && git config filter.evil.smudge CMD，全部在读写挂载内即可完成），isFile() 就为 false，整个门块被跳过，宿主侧的 checkout --force / clean -ffdx 随即在攻击者的仓库上执行。流水线在挂载之内从不创建这种形态——探针树总是由 git worktree add 产生（gitfile）——因此拒绝它没有误报成本；diff 中「普通 checkout 有 .git 目录」的说法针对的是未挂载状态，而那里 mountRootFor 返回 null，此守卫本就不会触发。
证据（用真实 git 走一遍真实的 restore spawn）：攻击前 .git 是 gitfile；攻击后「marker file created by smudge: YES」。（-c core.hooksPath=/dev/null/no-hooks -c core.fsmonitor= 只中和 hooks 与 fsmonitor，并不中和 filter。）
修复建议：在 isFile() 分支之前加入上方代码块中的守卫——挂载存在时，拒绝不是 gitfile 的 .git（流水线在挂载内从不创建的形态）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // reads that location any more — but it would sit in the mounted directory | ||
| // forever, because the sweep still skips the lease shape. Remove the one | ||
| // this call supersedes, and only that one. | ||
| rmSync(legacyLeasePath(repositoryRoot, params.target), { force: true }); |
There was a problem hiding this comment.
[Critical] R1-3: The legacy-lease removal added here is non-recursive, and force: true only suppresses ENOENT. If the legacy lease path is a directory, rmSync throws ERR_FS_EISDIR out of createReviewWorktreeLease and fetch-pr fails. The legacy path .qwen/tmp/qwen-review-lease-pr-<N>.json sits in the one directory reviewed code can still write, and nothing ever removes such a directory: fetch-pr's rollback rethrows without touching it, the cleanup sweep explicitly skips lease-shaped names (isReviewLeaseFile), and CI's heal rm -f cannot remove a directory. A reviewed PR's sandboxed suite only needs mkdir .qwen/tmp/qwen-review-lease-pr-42.json; from then on every fetch-pr for PR 42 on that machine throws EISDIR — a permanent wedge on that PR until an operator hand-deletes the directory. This is the exact wedge shape the diff's own paths.ts comment says the lease move eliminates ("plant one no session will ever sweep and wedge that PR on that machine"), re-opened on a writable path by this line. The sibling sweep in cleanup.ts documents an identical historical EISDIR and deliberately uses { recursive: true, force: true }.
Witness (probe driving the real function; flip check passes):
PROBE legacy-dir: createReviewWorktreeLease THREW ERR_FS_EISDIR: Path is a directory: rm returned EISDIR (is a directory) .../qwen-review-lease-pr-42.json
PROBE new-location lease written: false
with { recursive: true, force: true }: acquisition succeeded, new-location lease written
| rmSync(legacyLeasePath(repositoryRoot, params.target), { force: true }); | |
| rmSync(legacyLeasePath(repositoryRoot, params.target), { recursive: true, force: true }); |
中文说明
此处新增的旧位置 lease 删除是非递归的，而 force: true 只抑制 ENOENT。如果旧 lease 路径是一个目录，rmSync 会从 createReviewWorktreeLease 抛出 ERR_FS_EISDIR，导致 fetch-pr 失败。旧路径 .qwen/tmp/qwen-review-lease-pr-<N>.json 恰好位于被审代码仍可写入的那个目录里，而且没有任何机制会删除这样的目录：fetch-pr 的回滚只是重新抛出异常而不处理它，清理清扫明确跳过 lease 形状的文件名（isReviewLeaseFile），CI 的 heal 步骤 rm -f 也删不掉目录。被审 PR 的沙箱套件只需要执行 mkdir .qwen/tmp/qwen-review-lease-pr-42.json；从那一刻起，该机器上针对 PR 42 的每一次 fetch-pr 都会抛 EISDIR——该 PR 被永久卡住，直到有人手工删除这个目录。这正是 diff 自己的 paths.ts 注释声称 lease 迁移要消除的卡死形态（「植入一个无人会清扫、能把该 PR 在该机器上卡死的文件」），却被这一行在一个可写路径上重新打开。cleanup.ts 里的姊妹清扫记录过完全相同的历史 EISDIR，并刻意使用了 { recursive: true, force: true }。
证据（对真实函数的探针；翻转检查通过）：目录形态下 createReviewWorktreeLease 抛出 ERR_FS_EISDIR，新位置 lease 未写入；改为 { recursive: true, force: true } 后获取成功、新位置 lease 写入。
修复见上方 suggestion 代码块。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * One directory over is the whole fix: nothing mounts it, and the lease | ||
| * lifecycle is unchanged. | ||
| */ | ||
| export const REVIEW_LEASE_DIR = join('.qwen', 'review-leases'); |
There was a problem hiding this comment.
[Critical] R1-4: The lease move strands the pipeline's only automated orphan-lease sweeper: the if: always() "Clean review worktrees" heal step of the review-pr job (.github/workflows/qwen-code-pr-review.yml, near line 1974) still runs rm -f .qwen/tmp/qwen-review-lease-pr-*.json — the OLD location only. Nothing sweeps the new .qwen/review-leases location: cleanupReviewWorktreeLeases removes only leases matching the caller's own sessionId AND promptId, and no TTL/staleness heal exists. A review-pr job cancelled, timed out, or crashed before clearReviewWorktreeLease runs — the exact case the heal step's own comment names — leaves a dead lease at .qwen/review-leases/qwen-review-lease-pr-<N>.json on the self-hosted runner's persisted workspace. The next review of PR N reads it (fetch-pr.ts:828), reviewLeaseHeldByAnotherSession throws because every new session has a different session id, the job fails, and its heal step's glob matches nothing at the new location — every subsequent attempt re-fails identically, a permanent wedge on that PR until a human deletes the file by hand.
Witness (extract-step run of the real heal script against a fixture holding a dead lease at BOTH locations):
--- old location: (empty — swept)
--- new location:
qwen-review-lease-pr-77.json (survived)
Suggested fix (update the heal step in this PR; keep the old line for a grace period so pre-move strays keep sweeping):
rm -f .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true
rm -f .qwen/review-leases/qwen-review-lease-pr-*.json 2>/dev/null || true中文说明
lease 迁移使流水线中唯一的自动孤儿 lease 清扫器失效：review-pr 作业的 if: always() 「Clean review worktrees」修复步骤（.github/workflows/qwen-code-pr-review.yml，约 1974 行）仍然只执行 rm -f .qwen/tmp/qwen-review-lease-pr-*.json——只清扫旧位置。新位置 .qwen/review-leases 无人清扫：cleanupReviewWorktreeLeases 只删除与调用者自身 sessionId 和 promptId 都匹配的 lease，也不存在 TTL/过期修复机制。一旦 review-pr 作业在 clearReviewWorktreeLease 运行前被取消、超时或崩溃——正是该修复步骤自己的注释所指的场景——就会在自托管 runner 的持久工作区留下一个死 lease：.qwen/review-leases/qwen-review-lease-pr-<N>.json。下一次审查 PR N 时读到它（fetch-pr.ts:828），由于每个新会话的 session id 都不同，reviewLeaseHeldByAnotherSession 抛出异常，作业失败，而其修复步骤的通配符在新位置什么也匹配不到——之后每次尝试都以同样方式失败，该 PR 被永久卡住，直到有人手工删除该文件。
证据（用 extract-step 运行真实的修复脚本，夹具在新旧两个位置各放一个死 lease）：旧位置被清空，新位置的 qwen-review-lease-pr-77.json 幸存。
修复建议：在本 PR 中更新修复步骤，在现有行旁边加上新位置的 rm -f（保留旧位置一行一段宽限期，以便继续清扫迁移前的残留）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // the way — and the `checkout --force` below then runs whatever | ||
| // `filter.<x>.smudge` that directory declares, on the HOST. See | ||
| // `adminEntryInsideReviewTmp`. | ||
| if (adminEntryInsideReviewTmp(gitDir, mountRootFor, probeTree)) { |
There was a problem hiding this comment.
[Critical] R1-13: This new location gate protects only restoreProbeTreeTracked's checkout/clean pair. The revert phase's host-side git(probeTree, 'checkout', base, '--', ...modified) (test-efficacy.ts, near line 2995) is a SECOND host-side git write against the same mounted probe tree with no location gate at all — and the flow provably reaches it after this gate refuses: runOneMutant/runOneHunkProbe turn a restore refusal into verdict: 'inconclusive' without throwing, the mutation-phase catch explicitly continues "so the revert probe below still runs", and the revert phase's only guard is probeTargetEscapes — a symlink lstat check a rewritten gitfile trivially passes. Unlike resetScratchTree's shape, the planted entry needs no commondir file here — nothing in the revert path compares common dirs — so its own config is read normally. Probe trees are reused across reviews, so the tamper can also survive into the next review's revert checkout.
Witness (end-to-end probe driving the real command handler):
Arm A (PR as-is): gate refusal recorded as verdict: "inconclusive", revert probe shipped verdict: "inert" while escaped: true — marker file created by the smudge during git checkout base -- …
Arm B (same gate re-run before the revert checkout): escaped: false, revert phase refused
Fix: gate the revert checkout the same way — re-run the identity chain (or call restoreProbeTreeTracked) immediately before the revert checkout, or abort the probe run when a restore failed on tampering rather than staleness.
中文说明
这道新的位置门只保护 restoreProbeTreeTracked 的 checkout/clean 组合。revert 阶段宿主侧的 git(probeTree, 'checkout', base, '--', ...modified)（test-efficacy.ts，约 2995 行）是对同一挂载探针树的第二次宿主侧 git 写入，却完全没有位置门——而且流程可以确定地在上述门拒绝之后到达它：runOneMutant/runOneHunkProbe 把 restore 的拒绝变成 verdict: 'inconclusive' 而不抛异常，变异阶段的 catch 明确继续执行「以便下方的 revert 探针仍然运行」，而 revert 阶段唯一的守卫是 probeTargetEscapes——一个只查符号链接的 lstat 检查，被改写的 gitfile 轻松通过。与 resetScratchTree 的形态不同，这里植入的条目不需要 commondir 文件——revert 路径上不比较 common dir——因此它自己的 config 会被正常读取。探针树跨审查复用，因此篡改也可能存活到下一次审查的 revert checkout。
证据（驱动真实命令处理器的端到端探针）：Arm A（PR 原样）——门的拒绝被记录为 verdict: "inconclusive"，revert 探针随后给出 verdict: "inert"，而 escaped: true——marker 文件由 git checkout base -- … 期间的 smudge 创建；Arm B（在 revert checkout 前重跑同一道门）——escaped: false，revert 阶段被拒绝。
修复：用同样的方式守卫 revert checkout——在 revert checkout 之前重跑身份链（或调用 restoreProbeTreeTracked），或在 restore 因篡改（而非陈旧）失败时中止整个探针运行。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // the way — and the `checkout --force` below then runs whatever | ||
| // `filter.<x>.smudge` that directory declares, on the HOST. See | ||
| // `adminEntryInsideReviewTmp`. | ||
| if (adminEntryInsideReviewTmp(gitDir, mountRootFor, probeTree)) { |
There was a problem hiding this comment.
[Critical] R1-14: The FIRST host-side, filter-executing git write of the probe phase is the probe tree's own creation — git(worktree, 'worktree', 'add', '--detach', probeTree, headSha) (test-efficacy.ts, near line 2668) — which resolves the repository through the review worktree's OWN gitfile: a second rewritable pointer inside the same RW mount that no gate, new or existing, ever validates (test-efficacy.ts has no localFilterCommands-equivalent screen; that screen exists only in scratch-tree.ts). The build/test phase runs the PR's code in the container BEFORE the probe phase, and the container bind-mounts .qwen/tmp read-write: that code rewrites .qwen/tmp/review-pr-<N>/.git to gitdir: <abs>/.qwen/tmp/.evil carrying a config with filter.evil.smudge, a HEAD detached at the real sha, and objects/info/alternates naming the real objects dir; a committed one-line .gitattributes (* filter=evil) is harmless against the real repo's config, which defines no such filter. Host-side rev-parse HEAD answers through the rewritten gitfile (the planted real sha passes every coherence read), and the unguarded worktree add checks out through the planted config — executing the smudge payload on the HOST before this gate ever runs (it fires first inside restoreProbeTreeTracked, strictly later). Risk & Scope defers only "scratch-tree setup, base-tree build" — not probe-tree creation.
Witness (probe in reconstructed pipeline geometry; flip check passes):
ARM 1 (rewritten gitfile): rev-parse HEAD == real head sha; git worktree add --detach → exit 0; pwned.log: "PWNED-by-smudge … file=…/review-pr-1-probe" ×2 — payload executed ON THE HOST once per checked-out file
ARM 2 (honest gitfile restored): worktree add exit 0, FLIP OK: no pwned.log
Fix: before the creation spawn, require <worktree>/.git to be a plain file whose admin entry realpaths OUTSIDE mountRootFor(worktree) — reuse adminEntryInsideReviewTmp with the worktree's rev-parse --git-dir result — and refuse the phase otherwise.
中文说明
探针阶段第一个会执行 filter 的宿主侧 git 写入，是探针树自身的创建——git(worktree, 'worktree', 'add', '--detach', probeTree, headSha)（test-efficacy.ts，约 2668 行）——它通过审查工作树自己的 gitfile 解析仓库：这是同一个读写挂载之内第二个可被改写的指针，而没有任何门（无论新旧）校验过它（test-efficacy.ts 没有 localFilterCommands 等价的筛查；那道筛查只存在于 scratch-tree.ts）。构建/测试阶段在探针阶段之前就在容器里运行 PR 的代码，而容器以读写方式绑定挂载 .qwen/tmp：该代码把 .qwen/tmp/review-pr-<N>/.git 改写为 gitdir: <abs>/.qwen/tmp/.evil，并在那里放置带 filter.evil.smudge 的 config、指向真实 sha 的分离 HEAD、以及指向真实对象目录的 objects/info/alternates；提交的一行 .gitattributes（* filter=evil）对真实仓库的 config 无害——它没有定义这个 filter。宿主侧的 rev-parse HEAD 经由被改写的 gitfile 应答（植入的真实 sha 通过所有一致性读取），随后无守卫的 worktree add 透过植入的 config 执行 checkout——在这道门运行之前（它最早也要到 restoreProbeTreeTracked 内部才触发，严格更晚）就在宿主上执行了 smudge 载荷。Risk & Scope 只推迟了「scratch-tree 建立、base-tree 构建」——不包括探针树的创建。
证据（在重建的流水线几何中的探针；翻转检查通过）：ARM 1（改写的 gitfile）——rev-parse HEAD == 真实 head sha；git worktree add --detach → exit 0；pwned.log：「PWNED-by-smudge … file=…/review-pr-1-probe」×2——载荷在宿主上每个被检出的文件执行一次；ARM 2（恢复诚实的 gitfile）——worktree add exit 0，FLIP OK：无 pwned.log。
修复：在创建 spawn 之前，要求 <worktree>/.git 是普通文件，且其管理条目 realpath 位于 mountRootFor(worktree) 之外——用工作树的 rev-parse --git-dir 结果复用 adminEntryInsideReviewTmp——否则拒绝该阶段。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // the way — and the `checkout --force` below then runs whatever | ||
| // `filter.<x>.smudge` that directory declares, on the HOST. See | ||
| // `adminEntryInsideReviewTmp`. | ||
| if (adminEntryInsideReviewTmp(gitDir, mountRootFor, probeTree)) { |
There was a problem hiding this comment.
[Suggestion] R1-8: The sole production call site of this new gate is exercised by no test. Every existing restoreProbeTreeTracked fixture builds its probe tree via mkdtempSync(join(tmpdir(), ...)) — never containing a /.qwen/tmp/ segment — so mountRootFor returns null and the gate short-circuits to false before its planted-entry logic can run; the #9723 attack shape is replayed only at predicate level in worktree.test.ts, never through this wiring. Deleting this whole block, passing the wrong mount-root function, swapping the gitDir/probeTree arguments, or moving the check after the checkout --force spawn all ship green while the host-execution route reopens silently. Suggested fix: add one test that builds a real linked worktree under a .qwen/tmp layout, rewrites its gitfile at a planted admin entry under the same mount, and asserts the restore is refused before any checkout runs — plus an honest-entry control asserting the real admin entry still restores.
中文说明
这道新门的唯一生产调用点没有任何测试覆盖。现有的每个 restoreProbeTreeTracked 夹具都通过 mkdtempSync(join(tmpdir(), ...)) 构建探针树——从不含 /.qwen/tmp/ 段——因此 mountRootFor 返回 null，门在其植入条目逻辑运行之前就短路为 false；#9723 的攻击形态只在 worktree.test.ts 的谓词层面重放过，从未经过这段接线。删除整个门块、传错 mount-root 函数、交换 gitDir/probeTree 参数、或把检查挪到 checkout --force spawn 之后，整套测试都会绿灯通过，而宿主执行路径会悄无声息地重新敞开。修复建议：新增一个测试，构建 .qwen/tmp 布局下的真实 linked worktree，将其 gitfile 改写为同一挂载下植入的管理条目，断言 restore 在任何 checkout 运行之前被拒绝——再加一个诚实条目对照，断言真实管理条目仍能正常恢复。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // reads that location any more — but it would sit in the mounted directory | ||
| // forever, because the sweep still skips the lease shape. Remove the one | ||
| // this call supersedes, and only that one. | ||
| rmSync(legacyLeasePath(repositoryRoot, params.target), { force: true }); |
There was a problem hiding this comment.
[Suggestion] R1-9: The legacy-lease removal added here has zero test coverage, and its ordering contradicts the "only that one" comment on the EEXIST path. Untested branches: (a) a pre-move lease for the same target is removed on successful acquisition; (b) scoping — a legacy lease for pr-2 must survive acquiring pr-1; (c) the removal runs BEFORE the atomic wx write below, so when that write throws EEXIST (another session won between the gate's read and write), this call has already deleted a legacy file it did not supersede. That is unobservable within a single build but becomes observable through a pre-move reader during the transition window. Suggested fix: seed legacy-path leases in review-worktree-lease.test.ts and assert removal-on-acquire, other-target survival, and the intended EEXIST behaviour; if "only that one" is the spec, move the rmSync after the successful write.
中文说明
此处新增的旧位置 lease 删除没有任何测试覆盖，且其执行顺序在 EEXIST 路径上与「只删这一个」的注释相矛盾。未测试的分支：(a) 成功获取时，同一 target 的迁移前 lease 被删除；(b) 范围限定——pr-2 的旧 lease 必须在获取 pr-1 时幸存；(c) 删除在下方原子 wx 写入之前运行，因此当该写入抛出 EEXIST（另一个会话在门读取与写入之间胜出）时，这次调用已经删除了一个它并未取代的旧文件。这在单一构建内不可观测，但在过渡窗口中经由迁移前的读取方变得可观测。修复建议：在 review-worktree-lease.test.ts 中植入旧路径 lease，断言获取时删除、其他 target 幸存、以及预期的 EEXIST 行为；如果「只删这一个」是规范，把 rmSync 移到成功写入之后。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * One directory over is the whole fix: nothing mounts it, and the lease | ||
| * lifecycle is unchanged. | ||
| */ | ||
| export const REVIEW_LEASE_DIR = join('.qwen', 'review-leases'); |
There was a problem hiding this comment.
[Suggestion] R1-12: The move leaves two adjacent docstrings describing the pre-move layout: the LEASE_PREFIX comment in review-worktree-lease.ts says lease files live "under REVIEW_TMP_DIR", and isReviewLeaseFile's docstring says "a filename under REVIEW_TMP_DIR" — after this change leases are written under REVIEW_LEASE_DIR. The operator-facing refusal messages themselves print the correct new path via reviewLeasePath, so this misleads source readers rather than the error-message flow: a reader trusting the comment beside LEASE_PREFIX looks in .qwen/tmp, finds no live lease there, and concludes nothing is held while fetch-pr keeps refusing on the file sitting in .qwen/review-leases — a misdirected manual recovery instead of a one-step delete. Fix: update both docstrings to name REVIEW_LEASE_DIR (the isReviewLeaseFile one can note it is also still used to skip inert legacy files under REVIEW_TMP_DIR).
中文说明
这次迁移让相邻的两处文档字符串仍描述迁移前的布局：review-worktree-lease.ts 中 LEASE_PREFIX 的注释说 lease 文件位于「REVIEW_TMP_DIR 之下」，isReviewLeaseFile 的文档字符串说「REVIEW_TMP_DIR 下的文件名」——而这次变更之后 lease 写在 REVIEW_LEASE_DIR 之下。面向操作者的拒绝消息本身通过 reviewLeasePath 打印正确的新路径，因此误导的是源码读者而非报错流程：信任 LEASE_PREFIX 旁注释的读者会去 .qwen/tmp 找，找不到活跃 lease，便以为没有租约被持有，而 fetch-pr 却因 .qwen/review-leases 里的那个文件持续拒绝——本可一步删除的手工恢复被引向错误位置。修复：更新两处文档字符串以指明 REVIEW_LEASE_DIR（isReviewLeaseFile 处可注明它仍用于跳过 REVIEW_TMP_DIR 下的惰性遗留文件）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
@qwen-code /takeover stop |
qwen-code-dev-bot
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
Six blockers from round 1, all verified before fixing. **The predicate failed open at both boundaries of its own question.** An entry whose realpath IS the mount root produced an empty relative path and was read as outside — while the root is exactly as writable as anything under it — and a child legitimately named `..evil-git` produced a relative path starting with the characters an escape would. Both are places a planted entry can sit. The hand-rolled arithmetic is gone; `isSubpath` already answers this correctly and is already tested. **Every gate sat behind `.git` being a file.** Reviewed code that replaces the gitfile with a `.git` DIRECTORY of its own — `rm .git && git init .` inside the mount — skipped the lot, and the restore then ran against the repository it had just made. That shape is now refused before the branch, and only under a mount: outside one, a `.git` directory is what every ordinary checkout has. **The gate was on the second host-side write, not the first.** The probe tree's own creation — `git worktree add` — resolves the repository through the REVIEW worktree's gitfile, a second rewritable pointer in the same mount, written after the build phase has already run the PR's code in the container. `worktree add` checks files out, so it executes whatever filter that pointer leads to, before anything inside the restore could fire. Both call sites now ask the same question through one helper rather than two inline copies. **The legacy-lease removal was a wedge.** `force` only swallows ENOENT, so a DIRECTORY at the old lease path threw EISDIR out of acquisition — and `mkdir .qwen/tmp/qwen-review-lease-pr-42.json` is one command inside the mount, after which every review of that PR on that machine failed, with nothing able to remove it: the rollback rethrows, the sweep skips the lease shape, and `rm -f` cannot remove a directory. **The move stranded the only automated orphan sweeper.** The review job's `if: always()` heal step swept the old path only, so a cancelled or timed-out run left a dead lease at the new one, on a runner whose workspace persists. It now sweeps both, recursively. Tests follow the same rule they were missing: the gate's production call sites are driven, not just its predicate. Every previous fixture put its tree under a bare `tmpdir()`, where `mountRootFor` answers null and the gate short-circuits before its logic runs — which is how deleting the wiring shipped green. The new fixtures sit under `.qwen/tmp` and plant a COHERENT admin entry, because an incoherent one fails earlier for a reason that has nothing to do with the gate. Seven mutants, seven reds — including passing the probe tree where the review worktree belongs.
|
六条 Critical 全部处理， R1-1：我手写了路径包含判断，两个边界都 fail-open你说得对，而且这是最不该手写的东西。 已换成仓库里既有的 R1-2：所有身份门都在「
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6b": running npm run test:scripts to observe the pinning test failure live (no node_modules in the review worktree).
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 23883 passed.
中文说明
未探索到全部深度（达到工具调用预算）："agent 6b"：running npm run test:scripts to observe the pinning test failure live (no node_modules in the review worktree)。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 23883 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const untrusted = untrustedGitfile(probeTree, mountRootFor); | ||
| if (untrusted !== null) return untrusted; |
There was a problem hiding this comment.
[Critical] R1-13: Round 1's blocker on the revert phase is still open. The new location gate is wired into exactly two call sites — probe-tree creation (near line 2671) and the head of restoreProbeTreeTracked (this line) — but the revert phase's host-side git(probeTree, 'checkout', base, '--', ...modified) (near line 3003) is a THIRD host-side git write against the same mounted probe tree with no location gate; its only guard is probeTargetEscapes, a symlink-only lstat walk a rewritten gitfile trivially passes. Because runOneMutant/runOneHunkProbe turn a restore refusal into verdict: 'inconclusive' without throwing, and the mutation-phase catch explicitly continues "so the revert probe below still runs", the flow provably reaches the ungated checkout after every gate has refused.
Failure scenario: the PR's own test code — running while .qwen/tmp is writable — rewrites probeTree/.git to gitdir: <a standalone gitdir planted under the same mount> whose config carries filter.<x>.smudge, whose info/attributes marks the reverted files, and whose objects/info/alternates points at the real objects so base resolves. Every gated site then refuses (verdicts land inconclusive), and the revert checkout resolves the repository through the rewritten gitfile and applies the planted smudge filter on the host — outside the container, with the runner's tokens on CI. The git() helper passes no -c core.fsmonitor=, so a planted core.fsmonitor is a second execution vector on the same checkout.
Witness (end-to-end probe driving the real command handler at this commit; flips with the fix):
ARM A (PR as-is): marker written by the smudge during the revert checkout — R1-13-SMUDGE-RAN — after every gate had refused (verdicts: inconclusive)
ARM B (same untrustedGitfile gate added before the revert checkout): no marker
This also settles the "base cannot resolve in a planted repo" objection: alternates supply the objects, and the sha is data the attacker can read.
Fix: gate the revert checkout the same way — call untrustedGitfile(probeTree, mountRootFor) (or restoreProbeTreeTracked) immediately before the revert checkout, e.g. right after the probeTargetEscapes(probeTree, '.') check, recording the revert probe as inconclusive on refusal; or abort the probe run when a restore failed on tampering rather than staleness. Add a test that tampers the probe tree's gitfile after the gated restore and asserts the revert phase refuses.
中文说明
第 1 轮关于 revert 阶段的阻断项仍然敞开。新的位置门只接入了两个调用点——探针树创建（约 2671 行）与 restoreProbeTreeTracked 的开头（本行）——但 revert 阶段宿主侧的 git(probeTree, 'checkout', base, '--', ...modified)（约 3003 行）是对同一挂载探针树的第三次宿主侧 git 写入，没有任何位置门；其唯一守卫是 probeTargetEscapes——一个只查符号链接的 lstat 遍历，被改写的 gitfile 轻松通过。由于 runOneMutant/runOneHunkProbe 把 restore 的拒绝变成 verdict: 'inconclusive' 而不抛异常，且变异阶段的 catch 明确继续执行「以便下方的 revert 探针仍然运行」，流程可以确定地在所有门拒绝之后到达这道无守卫的 checkout。
失败场景：PR 自己的测试代码——在 .qwen/tmp 可写时运行——把 probeTree/.git 改写为指向同一挂载下植入的独立 gitdir，其 config 带 filter.<x>.smudge、info/attributes 标记被回退的文件、objects/info/alternates 指向真实对象目录使 base 可解析。随后每个有门的位置都拒绝（判定为 inconclusive），revert checkout 经由被改写的 gitfile 解析仓库，并在宿主上执行植入的 smudge filter——在容器之外，在 CI 上带着 runner 的令牌。git() 助手没有传 -c core.fsmonitor=，因此植入的 core.fsmonitor 是同一次 checkout 上的第二个执行向量。
证据（驱动真实命令处理器的端到端探针；应用修复后翻转）：ARM A（PR 原样）——在每道门都已拒绝（判定 inconclusive）之后，revert checkout 期间 smudge 写出了标记 R1-13-SMUDGE-RAN；ARM B（在 revert checkout 前加入同一道 untrustedGitfile 门）——无标记。这也回应了「base 无法在植入仓库中解析」的质疑：alternates 提供对象，而 sha 是攻击者可读的数据。
修复：用同样的方式守卫 revert checkout——在 revert checkout 之前调用 untrustedGitfile(probeTree, mountRootFor)（或 restoreProbeTreeTracked），例如紧跟 probeTargetEscapes(probeTree, '.') 检查之后，拒绝时把 revert 探针记为 inconclusive；或在 restore 因篡改（而非陈旧）失败时中止整个探针运行。并新增一个测试：在有门的 restore 之后篡改探针树的 gitfile，断言 revert 阶段拒绝。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| rm -rf .qwen/review-leases/qwen-review-lease-pr-*.json 2>/dev/null || true | ||
| rm -rf .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true |
There was a problem hiding this comment.
[Critical] The lease-sweep rewrite breaks the sync test that pins this exact workflow line. scripts/tests/review-worktree-cleanup-workflow.test.js (test: 'keeps the review-job cleanup sweep pinned to paths.ts') asserts expect(reviewCleanStep).toContain(rm -f ${toPosix(REVIEW_TMP_DIR)}/${LEASE_PREFIX}pr-*.json) — the literal substring rm -f .qwen/tmp/qwen-review-lease-pr-*.json. This diff replaced that line with two rm -rf lines; rm -f .qwen/tmp/... is not a substring of rm -rf .qwen/tmp/... (the inserted -r breaks the match), and these are the only occurrences of the lease glob in the step — so the spec suite is red at HEAD. The lane is npm run test:scripts, the trailing clause of root test:ci, run by ci.yml's required test job on every pull_request and merge_group: this PR's own required check fails on this. Secondarily, nothing pins the new .qwen/review-leases glob to REVIEW_LEASE_DIR/LEASE_PREFIX, so a later rename of the lease dir would silently no-op the new-location sweep — exactly the drift this spec exists to catch (its header cites a rename that once broke a sweeper).
Witness (live run at this commit):
npx vitest run --config ./scripts/tests/vitest.config.ts review-worktree-cleanup-workflow
Test Files 1 failed (1); Tests 1 failed | 14 passed (15)
AssertionError: expected 'set -uo pipefail…' to contain 'rm -f .qwen/tmp/qwen-review-lease-pr-…'
Fix (in the spec file, same PR): import REVIEW_LEASE_DIR alongside REVIEW_TMP_DIR and pin both new lines:
expect(reviewCleanStep).toContain(
`rm -rf ${toPosix(REVIEW_LEASE_DIR)}/${LEASE_PREFIX}pr-*.json`,
);
expect(reviewCleanStep).toContain(
`rm -rf ${toPosix(REVIEW_TMP_DIR)}/${LEASE_PREFIX}pr-*.json`,
);中文说明
lease 清扫的改写打破了钉住这条工作流行的同步测试。scripts/tests/review-worktree-cleanup-workflow.test.js（测试「keeps the review-job cleanup sweep pinned to paths.ts」）断言 expect(reviewCleanStep).toContain(rm -f ${toPosix(REVIEW_TMP_DIR)}/${LEASE_PREFIX}pr-*.json)——即字面子串 rm -f .qwen/tmp/qwen-review-lease-pr-*.json。本 diff 把那一行换成了两条 rm -rf 行；rm -f .qwen/tmp/... 不是 rm -rf .qwen/tmp/... 的子串（插入的 -r 破坏了匹配），而这两行是该步骤中 lease 通配符仅有的出现处——因此该规格套件在 HEAD 上是红的。该测试所在通道是 npm run test:scripts，即根目录 test:ci 的收尾环节，由 ci.yml 的必需 test 作业在每个 pull_request 与 merge_group 上运行：本 PR 自己的必需检查会因此失败。其次，新的 .qwen/review-leases 通配符没有任何东西钉到 REVIEW_LEASE_DIR/LEASE_PREFIX，日后重命名 lease 目录会悄无声息地让新位置的清扫失效——正是这份规格文件存在所要防的漂移（其开头就记录过一次重命名曾弄坏清扫器）。
证据（在本提交上实际运行）：npx vitest run --config ./scripts/tests/vitest.config.ts review-worktree-cleanup-workflow → Tests 1 failed | 14 passed——AssertionError: expected 'set -uo pipefail…' to contain 'rm -f .qwen/tmp/qwen-review-lease-pr-…'。
修复（在同一 PR 的规格文件中）：把 REVIEW_LEASE_DIR 与 REVIEW_TMP_DIR 一并导入，钉住两条新行（见上方英文部分的代码块）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // The FIRST host-side git write of this phase, and it resolves the | ||
| // repository through the REVIEW worktree's own gitfile — a second |
There was a problem hiding this comment.
[Suggestion] This comment claims worktree add is "The FIRST host-side git write of this phase", but discardWorktree(worktree, probeTree) three lines above already ran host-side git writes — git worktree remove --force and git worktree unlock (and in its fallback git rev-parse --git-common-dir), all with cwd: worktree — resolving the repository through the same review-worktree gitfile the gate exists to police, before the gate runs. The integration test's comment repeats the same claim. The ordering is safe in substance — those commands check nothing out, so no smudge filter or hook runs (verified) — but this is a false coverage statement in the rationale of a security gate: the next maintainer extending this block trusts "everything above is not a git write" and can place another git invocation before the gate on the strength of it. Reword to what is true — e.g. "the first host-side git write of this phase that CHECKS FILES OUT (discardWorktree's worktree remove/unlock above write no file content, so run no filter)" — or hoist the gate above discardWorktree so the sentence becomes literally true (the refusal aborts the whole create sequence anyway).
中文说明
这条注释声称 worktree add 是「本阶段第一次宿主侧 git 写入」，但其上方三行的 discardWorktree(worktree, probeTree) 已经执行过宿主侧 git 写入——git worktree remove --force 与 git worktree unlock（及其回退路径里的 git rev-parse --git-common-dir），全部以 cwd: worktree 运行——在该门生效之前就已通过这道门本要监管的同一个 review 工作树 gitfile 解析仓库。集成测试里的注释重复了同样的说法。顺序在实质上是安全的——那些命令不检出任何文件，因此不会执行 smudge filter 或 hook（已验证）——但这是安全门的理由陈述中一处虚假的覆盖声明：下一位扩展这段代码的维护者会相信「上方的一切都不是 git 写入」，并据此把另一次 git 调用放到门之前。改成符合事实的表述——例如「本阶段第一次检出文件的宿主侧 git 写入（上方 discardWorktree 的 worktree remove/unlock 不写文件内容，因此不执行 filter）」——或把门提到 discardWorktree 之上，使这句话字面成立（拒绝反正会中止整个创建序列）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const untrusted = untrustedGitfile(worktree, mountRootFor); | ||
| if (untrusted !== null) { | ||
| throw new Error(`refusing to create a probe tree: ${untrusted}`); |
There was a problem hiding this comment.
[Suggestion] The class-level invariant stated in untrustedGitfile's docstring — "Every host-side git command that WRITES resolves the repository through the .git of the tree it runs in" — is enforced per call site: exactly two of those sites carry the gate (this spawn and the head of restoreProbeTreeTracked), the module's git()/gitOut() chokepoint carries none, and restoreProbeTreeTracked uses raw spawnSync, so no shared layer closes the class. The cost is no longer hypothetical: the revert-phase checkout (R1-13) is a third write site this per-site discipline forgot, now probe-confirmed executing a planted filter on the host. Even at the gated sites the shape is check-then-use — the gate reads <tree>/.git, then git's discovery reads it again; the pipeline's own comments document concurrent shards against one common dir, so a concurrent session's sandboxed phase inside the same mount could rewrite the pointer in that window (undemonstrated, hence Suggestion). Consider resolving host-side git through host-trusted state instead of the rewritable pointer: the lease already carries repositoryRoot, so pinning --git-dir (and --work-tree for the restore) from it at these spawns removes the gitfile's authority entirely and collapses check-then-use to a single resolution. At minimum, centralise the gate in the module's git()/gitOut() helper so the class is closed once for every caller instead of remembered per call site.
中文说明
untrustedGitfile 文档字符串中陈述的类级不变量——「每一个执行写入的宿主侧 git 命令，都经由其所运行树的 .git 解析仓库」——目前是逐调用点强制的：这些写入点中恰好只有两处带门（本处 spawn 与 restoreProbeTreeTracked 的开头），模块的 git()/gitOut() 咽喉点没有任何门，restoreProbeTreeTracked 用的是裸 spawnSync，因此没有任何共享层把这个类关掉。代价已不再是假设：revert 阶段的 checkout（R1-13）正是这种逐点纪律漏掉的第三个写入点，现已由探针确认会在宿主上执行植入的 filter。即便在有门的位置，形态也是「先检查后使用」——门读取 <tree>/.git，随后 git 的发现过程再读一次；流水线自己的注释记录了多个分片并发作用于同一 common dir 的场景，同一挂载内另一个并发会话的沙箱阶段可以在该窗口内改写指针（未演示，故为 Suggestion）。考虑改为通过宿主可信状态解析宿主侧 git，而不是经由可改写的指针：lease 本就携带 repositoryRoot，在这些 spawn 处用它钉住 --git-dir（restore 再加 --work-tree），可以彻底移除 gitfile 的权威，并把「先检查后使用」收敛为一次解析。至少，应把门集中到模块的 git()/gitOut() 助手里，让这个类一次性地为所有调用者关闭，而不是靠每个调用点各自记得。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (adminEntryInsideReviewTmp(target, mountRoot, tree)) { | ||
| return `${tree}'s admin entry is inside the review temp dir, where the reviewed code can rewrite it`; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
[Suggestion] untrustedGitfile's ADMIT path under a mount — an intact pipeline gitfile resolving outside .qwen/tmp returns null and the run proceeds — has zero test coverage. No test imports untrustedGitfile; every pre-existing fixture builds its tree under a bare tmpdir() and short-circuits at mountRootFor === null; the two new mounted tests reach only refusal shapes (the .git-directory shape through runOneMutant, the rewritten-gitfile shape through runHandler). Consequence: any one-line mutation that corrupts the parsed target — e.g. target = resolve(tree, raw.slice('gitdir:'.length).trim().slice(1)) — survives the whole suite: the integration test's planted path gets mangled, realpathSync fails, adminEntryInsideReviewTmp fails closed, and the refusal the tests expect still happens — while in production the same mutation mangles every legitimate gitfile target and refuses every real review's probe phase ("refusing to create a probe tree: ..."), with the suite green. Add one positive case: a worktree created by git worktree add under <repo>/.qwen/tmp/... with its gitfile untouched, asserting untrustedGitfile(wt, mountRootFor) returns null — ideally also one end-to-end case under .qwen/tmp that completes instead of refusing, so the no-false-positive guarantee is pinned through the production call site.
中文说明
untrustedGitfile 在挂载之下的放行路径——完好的流水线 gitfile 解析到 .qwen/tmp 之外、返回 null 且运行继续——没有任何测试覆盖。没有测试文件导入 untrustedGitfile；现有的每个夹具都把树建在裸 tmpdir() 之下，在 mountRootFor === null 处短路；两个新的挂载夹具只到达拒绝形态（.git 目录形态经由 runOneMutant，改写 gitfile 形态经由 runHandler）。后果：任何一行破坏目标解析的变异——例如 target = resolve(tree, raw.slice('gitdir:'.length).trim().slice(1))——都能在整个套件下存活：集成测试植入的路径被弄坏，realpathSync 失败，adminEntryInsideReviewTmp 失败关闭，测试期望的拒绝照样发生——而在生产环境，同样的变异会弄坏每一个合法 gitfile 目标，拒绝每一次真实审查的探针阶段（「refusing to create a probe tree: ...」），而套件全绿。补一个正向用例：用 git worktree add 在 <repo>/.qwen/tmp/... 下创建工作树且不动其 gitfile，断言 untrustedGitfile(wt, mountRootFor) 返回 null——最好再补一个在 .qwen/tmp 下完整跑通（而非被拒绝）的端到端用例，把「无误报」的保证钉在生产调用点上。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…ns the sweep **The workflow edit broke its own pinning test, and this PR's required check with it.** `scripts/tests/review-worktree-cleanup-workflow.test.js` asserts the sweep line literally, derived from `paths.ts` so a rename there fails the build rather than silently no-op-ing the sweep on the runners. Adding `-r` broke the substring. The test now pins BOTH locations — the new one where this job's own runs leave leases, the old one where a persisted workspace can still hold one — which is what the test existed to enforce and what the move should have updated in the same breath. It went unnoticed because the commit was verified with vitest alone; `npm run test:scripts` is a separate lane of the same required job. **The revert phase was a third ungated write, reached exactly when the others refuse.** `git checkout base -- …` runs against the same mounted probe tree, and its only guard was a symlink-only lstat walk that a rewritten gitfile passes untouched. Worse, a restore refusal becomes `inconclusive` without throwing and the mutation phase's catch continues on purpose "so the revert probe below still runs" — so guarding the first two writes and not this one left the route open precisely where the other two closed it. **A claim in the gate's own rationale was false.** The comment called `worktree add` "the FIRST host-side git write of this phase"; `discardWorktree` above already runs `worktree remove --force` and `worktree unlock` with the same cwd. They materialise nothing, so no filter runs — but that distinction is the entire reason the gate can sit below them, and the looser sentence invites the next maintainer to add a checkout above it. Tests follow the same rule as the code. The ADMIT path had no coverage, so a mutation mangling the parsed gitdir survived: every refusal fixture would still refuse, because a mangled target fails to resolve and the location check fails closed — while in production the same mangling refuses every healthy tree. It is asserted now, relative spelling included, since git writes that pointer relative when it can. And the revert gate is driven through the phase itself, by a fake runner that rewrites `.git` from inside the probe tree — which is who does it in production. Nine mutants, nine reds.
|
四条全部处理， R2-1：我改工作流，破坏了钉住那一行的同步测试实测确认： 加了 现在它把两个位置都钉住：新位置是本 job 自己的运行留下 lease 的地方，旧位置是持久工作区上早期构建可能还留着的地方。 漏掉的原因值得记一笔：我提交前只跑了 vitest。 R1-13：revert 是第三个未设门的写入，而且恰恰在其他门拒绝之后才到这条最要紧，你的因果链我核过了：restore 的拒绝变成 给前两个写入设门却漏掉这一个，等于在其他两处刚关上的地方把路留着。已补，并且是通过该阶段本身驱动测试的：夹具里的假运行器在探针树里改写 R2-2：我在安全门的理由里写了一句不实的话
R2-4：放行路径无覆盖，你给的变异确实能存活也成立，而且理由很刁：所有拒绝用例在「篡改解析目标」的变异下仍然会拒绝——目标被弄坏就解析不了，位置检查 fail-closed，期望的拒绝照样发生——而生产里同一个变异会拒掉每一棵健康的树。只有断言放行才能把两者分开。已补，并且覆盖相对路径拼法（git 能用相对就用相对，所以解析必须相对于树而非进程 cwd）。变异（ R2-3「按调用点逐个设门关不掉这个类」我同意这是事实描述。这一轮把第三个点补上之后，三个会检出文件的宿主写入都设了门；但我没有去做共享层，因为 九个变异，九次红。review + lease 全部套件 104 文件 5085 通过， |
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 (merge_group only) and the new integration tests never ran on Windows — the red that lane carries is R3-1.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the new integration tests never ran on Windows — the red that lane carries is R3-1。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const out = JSON.parse( | ||
| readFileSync(join(repo, 'out.json'), 'utf8'), | ||
| ) as Record<string, unknown>; | ||
| expect(JSON.stringify(out)).toContain('review temp dir'); |
There was a problem hiding this comment.
[Critical] Both integration tests added this round are deterministically red on the test_windows merge-queue lane. The asserted refusal string review temp dir is only ever produced by untrustedGitfile's refusal messages, and every one of them sits below const root = mountRoot(tree); if (root === null) return null; — while on Windows mountRootFor refuses EVERY absolute path (drive-letter colon: if (real.includes(':')) return null;), so the gates never speak there: the run completes with ordinary verdicts, out.json never contains the string, and this assertion fails. The twin assertion in "refuses to revert through a gitfile the probe run rewrote" (line ~433) fails identically, and the generated fake-runner embeds ${planted} in a JS string literal without escaping backslashes (line ~408), which self-mangles the gitfile rewrite on Windows as well. The first red lands inside the merge queue: Test (windows-latest, Node 22.x) runs only under merge_group and reports as skipped on the PR page, so green here is no evidence — it runs npm run test:ci, whose packages/cli vitest run collects this file, and the failure ejects the queue entry and stalls every PR batched with it.
Witness (env-gated simulation of the documented win32 intermediate mountRootFor → null, flip-checked on the same scratch tree — verified against a local model of the win32 condition, not an actual Windows lane):
SIM=1: AssertionError: expected '{"unreachable":[],"probed":[…]}' to contain 'review temp dir' (both tests)
SIM unset, same tree: both tests pass
Wrap both it(...) declarations in it.skipIf(process.platform === 'win32') — the convention already used in this file (line ~1273), and the codebase's own sandboxed-exec.test.ts gates its mount assertions with the same skip for this exact reason. The production gate is genuinely inert on Windows by design (no containment, so no writable-surface threat), so gating the tests — not weakening the gate — is the correct fix.
中文说明
本轮新增的两个集成测试在 test_windows 合并队列通道上必然失败。被断言的拒绝串 review temp dir 只可能出自 untrustedGitfile 的拒绝消息，而这些消息全部位于 const root = mountRoot(tree); if (root === null) return null; 之下——而在 Windows 上 mountRootFor 拒绝每一个绝对路径（盘符冒号：if (real.includes(':')) return null;），因此那道门在那里从不发声：流程以普通判定结束，out.json 中不含该串，本断言失败。「refuses to revert through a gitfile the probe run rewrote」中的孪生断言（约 433 行）以同样方式失败；生成的假运行器把 ${planted} 原样嵌入 JS 字符串字面量而未转义反斜杠（约 408 行），在 Windows 上同样会把 gitfile 改写弄成乱码。首个红灯落在合并队列内：Test (windows-latest, Node 22.x) 只在 merge_group 下运行、在 PR 页面显示为已跳过，所以此处的绿灯不构成证据——它运行 npm run test:ci，其中 packages/cli 的 vitest run 会收集本文件，失败将把队列条目踢出并卡住同批的所有 PR。
证据（用环境变量门控模拟文档化的 win32 中间态 mountRootFor → null，在同一临时树上翻转验证——验证对象是 win32 条件的本地模型，而非真实 Windows 通道）：SIM=1 时两个测试都在该断言处失败；同一棵树未设该变量时两个测试都通过。
请把两个 it(...) 声明包上 it.skipIf(process.platform === 'win32')——本文件已有此惯例（约 1273 行），且 sandboxed-exec.test.ts 自己也以同样的跳过门控其挂载断言。生产门在 Windows 上按设计本就惰性（无容器化即无可写面威胁），因此门控测试——而非削弱生产门——才是正确的修法。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // The fixture has to sit under `.qwen/tmp`: everywhere else `mountRootFor` | ||
| // answers null and the gate short-circuits, which is how deleting it | ||
| // shipped green. | ||
| write('package.json', '{"private":true,"workspaces":["packages/*"]}\n'); | ||
| write('packages/lib/src/f.ts', 'export const f = () => 1;\n'); | ||
| const base = commitAll('base'); | ||
| write('packages/lib/src/f.ts', 'export const f = () => 2;\n'); | ||
| write( | ||
| 'packages/lib/src/f.test.ts', | ||
| 'import { f } from "./f.js"; import { it, expect } from "vitest"; it("t", () => expect(typeof f).toBe("function"));\n', | ||
| ); | ||
| commitAll('pr'); | ||
| const wt = join(repo, '.qwen', 'tmp', 'review-pr-1'); | ||
| mkdirSync(dirname(wt), { recursive: true }); | ||
| git(repo, 'worktree', 'add', '-q', '--detach', wt, 'HEAD'); |
There was a problem hiding this comment.
[Suggestion] Both tests added this round paste the ~22-line fixture that scaffoldModifiedPr() (same file, line 91) already provides — the only difference is the worktree location (join(repo, 'wt') vs join(repo, '.qwen', 'tmp', 'review-pr-1')) — so the fixture now exists in three copies that must evolve in lock-step: when its shape changes (the report.json schema, the workspace layout the handler requires, the vitest fake contract), the fix must land in three places, and missing one of the two gate-test copies leaves a security-gate test red for a reason unrelated to the gate, or green while exercising a fixture shape the pipeline no longer produces.
Parameterize the helper instead of forking it: function scaffoldModifiedPr(wtPath: string = join(repo, 'wt')) doing mkdirSync(dirname(wtPath), { recursive: true }) + git(repo, 'worktree', 'add', '-q', '--detach', wtPath, 'HEAD'), then call it as const { wt, base } = scaffoldModifiedPr(join(repo, '.qwen', 'tmp', 'review-pr-1'));. The location difference is deliberate (the gate only fires under .qwen/tmp) — which is exactly why it belongs as a parameter, not a fork.
中文说明
本轮新增的两个测试都粘贴了 scaffoldModifiedPr()（同文件第 91 行）已经提供的约 22 行夹具——唯一差别是工作树位置（join(repo, 'wt') 对比 join(repo, '.qwen', 'tmp', 'review-pr-1')）——于是该夹具现在有三份必须同步演进的副本：一旦其形状变化（report.json 模式、处理器要求的工作区布局、vitest 假运行器契约），修复必须落在三处，而漏掉两个门测试副本中的任意一个，就会让安全门测试因与门无关的原因变红，或在流水线已不再产生的夹具形状上保持绿色。
请把该助手参数化而不是复制分叉：function scaffoldModifiedPr(wtPath: string = join(repo, 'wt'))，内部执行 mkdirSync(dirname(wtPath), { recursive: true }) 与 git(repo, 'worktree', 'add', '-q', '--detach', wtPath, 'HEAD')，然后以 const { wt, base } = scaffoldModifiedPr(join(repo, '.qwen', 'tmp', 'review-pr-1')); 调用。位置差异是刻意的（门只在 .qwen/tmp 下触发）——正因如此它应成为参数，而不是分叉。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…annot exist The three tests added for this gate are deterministically red on Windows, and the reason is this PR's own sibling: `mountRootFor` refuses every absolute Windows path, because a drive letter is a colon and the `-v` grammar has one separator. Containment is unavailable there by design, so the gates never speak, `out.json` never carries the refusal, and the assertions fail for a reason that has nothing to do with what they test. The Windows lane runs only under `merge_group` and reports as skipped on the PR page, so green here was no evidence — the first red would have landed in the queue and stalled the batch. Gated with the same named predicate the sibling suite uses, so the reason is stated once rather than repeated three times. Two more from the same review. The fake runner embedded the planted path in a JS string literal by interpolation, which a Windows path's backslashes self-mangle; it is generated through `JSON.stringify` now. And both new tests had pasted the ~22-line fixture that `scaffoldModifiedPr` already provides, leaving three copies to evolve in lock-step — the helper takes the worktree location as a parameter instead, since the location is exactly what differs and exactly why the gates fire. The three gates were re-mutated after the refactor, one at a time: three mutants, three reds.
|
两条都处理了， R3-1：Windows 腿——同一个教训我第二次踩成立，而且成因正是本 PR 的姊妹改动： 而且 Windows 腿只在 这条我在 #9723 上已经被指出过一次同类问题（那次是 同一条里的第三点也修了：假运行器把植入路径用插值塞进 JS 字符串字面量，Windows 路径的反斜杠会自我损坏——现在用 R3-2：夹具三份拷贝也成立，而且你给的处方比我原来的做法好：位置差异恰恰是门是否开火的原因，所以它属于参数而不是分叉。 重构之后重新验证参数化和平台门都可能悄悄削弱防护，所以三道门在重构后逐个重新变异（创建 / restore / revert），三个变异三次红。 review + lease 全部套件 104 文件 5085 通过； |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory.
Convergence: round 4 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/review/test-efficacy.integration.test.ts (findings in round 3; 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. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory。
收敛情况：第 4 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/test-efficacy.integration.test.ts（第 3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ) as Record<string, unknown>; | ||
| expect(JSON.stringify(out)).toContain('review temp dir'); |
There was a problem hiding this comment.
[Suggestion] Test 1's oracle cannot pin the gate-before-worktree add ordering its describe name states. worktreeCreateFailureDetail serializes the creation gate's refusal message identically whichever side of the worktree add spawn the throw fires on, so toContain('review temp dir') passes for refuse-then-add (today's safe order) and for add-then-refuse alike.
Failure scenario: a maintainer refactoring the probe setup moves git(worktree, 'worktree', 'add', '--detach', probeTree, headSha) above the untrustedGitfile check (test-efficacy.ts:2679-2684) — the production comment there explicitly warns that a checkout added above the gate reopens the route. worktree add then resolves the repository through the planted .evil-git pointer and checks files out on the host — executing any planted filter.<x>.smudge — before the gate throws the same message; out.json carries the identical detail and this test, the only one pinning the creation gate, stays green while the host-execution route is open.
Witness (probe in a scratch tree, flip protocol):
gate first, no fix → Tests 1 passed | 31 skipped
spawn moved above gate → Tests 1 passed | 31 skipped (mutant survives)
mutant + existsSync fix → AssertionError: expected true to be false
original order + fix → green (no false positive)
The handler's failure path never discards a partially-created probe tree (the discard is guarded by created, which stays false when the gate throws), so the assertion below is sound. Optionally also assert that no probe phase ran (a single probed record, no inert verdicts) to cover the stronger variant that moves created = true above the gate as well.
| ) as Record<string, unknown>; | |
| expect(JSON.stringify(out)).toContain('review temp dir'); | |
| ) as Record<string, unknown>; | |
| expect(JSON.stringify(out)).toContain('review temp dir'); | |
| expect(existsSync(`${wt}-probe`)).toBe(false); |
中文说明
测试 1 的断言无法钉住其 describe 名所声明的「门先于 worktree add」这一顺序属性。worktreeCreateFailureDetail 对创建门拒绝消息的序列化，在 throw 发生在 worktree add 之前还是之后完全相同，因此 toContain('review temp dir') 在「先拒绝后 add」（今天的安全顺序）与「先 add 后拒绝」两种情况下都通过。
失败场景：维护者重构探针建立流程时，把 git(worktree, 'worktree', 'add', '--detach', probeTree, headSha) 移到 untrustedGitfile 检查（test-efficacy.ts:2679-2684）之上——该处生产代码注释已明确警告：在门之上添加 checkout 会重新打开这条逃逸路线。届时 worktree add 会经由植入的 .evil-git 指针解析仓库，并在宿主上检出文件——执行植入的 filter.<x>.smudge——之后门才抛出相同的消息；out.json 中是完全相同的 detail，而这个唯一钉住创建门的测试保持绿色，宿主执行逃逸路线却已敞开。
证据（在临时树中的探针，翻转协议）：门在前且未加修复 → 1 passed；把 spawn 移到门之上 → 仍 1 passed（变异体存活）；变异体 + 下方 existsSync 修复 → AssertionError: expected true to be false；原顺序 + 修复 → 绿色（无误报）。
处理器的失败路径不会丢弃部分创建的探针树（丢弃由 created 守卫，而门抛出时它仍为 false），因此下方断言是稳妥的。可选地再断言探针阶段根本未运行（probed 只有一条记录、无 inert 判定），以同时覆盖把 created = true 也移到门之上的更强变体。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Both gate tests asserted a refusal MESSAGE, and `worktreeCreateFailureDetail` serializes that message identically whether the gate throws before the write or after it. So the mutant that matters — moving `worktree add` above the gate, which is exactly what the comment beside it warns a refactor might do — checked files out through the planted pointer, ran its filters on the host, and left both tests green. Creation: the probe tree not existing is what says the spawn never ran. Revert: the planted repository now carries a real `filter.evil.smudge`, and the assertion is that its canary was never written. That is the property itself rather than a proxy for it — with the gate removed the filter EXECUTES on the host inside the suite, which is what the test now catches. The fixture had to become coherent for this to mean anything: an empty planted directory fails the checkout for reasons unrelated to the gate, so it copies the real admin entry and points `commondir` at a planted common dir, the shape the reproduction in the PR description uses.
|
你说对了：消息判据分不清顺序
创建那条：判据改成探针树不存在，这才说明 spawn 没跑过。变异（交换两行顺序）→ 红（此前存活）。 revert 那条我做得更进一步它有同样的弱点，而我发现可以给它一个直接判据而不是代理判据：让植入的库带上真的 这不是「出现了某条消息」，而是「没有发生宿主执行」——属性本身。为此夹具必须变得自洽（空目录会因为与门无关的原因让检出失败），所以它现在复制真实管理条目、把 删掉 revert 门之后，那个 smudge filter 在测试套件内真的在宿主上执行了（canary 出现）。这条测试现在抓的就是这件事。 顺带你报的 结合你前几轮披露的「review worktree 里没有 node_modules」，这应该是评审环境而不是命令本身。 review + lease 全部套件 104 文件 5085 通过。tsc / lint / prettier 干净。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 5": executing the new/changed vitest suites to confirm green (no node_modules in the review worktree; install + build exceeds session budget).
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 24748 passed.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/worktree.ts:147 — [review] Two fail-closed refusal branches of untrustedGitfile have zero test coverage; a flip to admit survives the whole suite
Convergence: round 5 posted 2 inline comment(s), 2 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, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
未探索到全部深度（达到工具调用预算）："agent 5"：executing the new/changed vitest suites to confirm green (no node_modules in the review worktree; install + build exceeds session budget)。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 24748 passed。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (!raw.startsWith('gitdir:')) { | ||
| return `${tree}'s .git does not name an admin entry`; | ||
| } | ||
| target = resolve(tree, raw.slice('gitdir:'.length).trim()); |
There was a problem hiding this comment.
[Critical] R5-1: the gate resolves the gitfile with its own JS arithmetic, and git resolves the same file differently — two entrances probed end-to-end at this commit, and the divergence class has no last corner.
untrustedGitfile parses gitdir: and resolves the target in Node; the host-side write that follows resolves the same file in git. Wherever the two resolvers disagree, the gate admits a pointer whose real referent only git sees, and the gated write checks out through it. Both shapes were driven through the real function (git 2.43.0) with flip checks:
- Whitespace class — JS
String.prototype.trim()strips U+00A0 (and U+FEFF, U+1680, U+2000–U+200A, U+202F, U+205F, U+3000); git'sread_gitfiletrims only C-localeisspacebytes. Reviewed code — which the sandbox lets write anywhere under.qwen/tmp— rewrites the worktree's.gittogitdir: <U+00A0><repo>/.git/worktrees/review-pr-1and plants a coherent admin entry (backpointer,commondir,filter.<x>.smudge) at<tree>/<U+00A0><repo>/.git/worktrees/review-pr-1, inside the mount. The gate trims the NBSP, resolves the REAL entry (outside the mount, exists), admits; git resolves the planted entry; the gated write executes the smudge on the host. - Resolution-base class —
resolve(tree, target)is lexical, but a spawned git resolves a relative target against the tree's PHYSICAL path afterchdirfollows a symlink. With the review worktree replaced by a symlink inside the mount pointing at a deeper attacker dir whose gitfile uses a relative target, the gate admits while git lands on a planted admin entry under the mount.
Witness (probes at this commit):
Entrance 1: {"gate": null, "gitAbsoluteGitDir": ".../.qwen/tmp/review-pr-1/\u{a0}.../.git/worktrees/review-pr-1",
"worktreeAdd": "succeeded", "canaryWritten": true}
flip (C-locale trim): gate refuses
Entrance 2: GATE VERDICT: ADMIT (null); GIT RESOLVES GIT DIR TO: .../repo/.qwen/tmp/.git/worktrees/review-pr-1;
GIT-RESOLVED DIR INSIDE MOUNT: YES; worktree add exit: 0; CANARY CREATED: smudge EXECUTED ON HOST
flip (leaf-symlink refusal): GATE VERDICT: REFUSE, canary absent
The entrance space — every byte and path shape git resolves differently than the JS model — cannot be enumerated and closed corner by corner, so per-character patches here will not converge. Close it structurally: classify the location git itself resolves — spawn git rev-parse --absolute-git-dir in the tree under sanitizedGitEnv() (this flow already runs rev-parse reads in these trees), and refuse on spawn failure or when the realpath sits inside the mount root. The gate's decision and the subsequent write are then about the same path by construction, which closes the whitespace/encoding class and the lexical-vs-physical class at once. Add both shapes to the untrustedGitfile test block.
中文说明
[Critical] R5-1：这道门用 JS 自己的路径算术解析 gitfile，而 git 对同一文件的解析结果不同——本提交上已端到端实测两种入口，且该分歧类别没有最后一个角落。
untrustedGitfile 在 Node 里解析 gitdir: 并求出目标路径；紧随其后的宿主侧写入则由 git 解析同一个文件。只要两个解析器不一致，门就会放行一个「真实指向只有 git 看得到」的指针，而被守卫的写入会透过它检出文件。两种形态都已驱动真实函数复现（git 2.43.0），且翻转检查通过：
- 空白字符类——JS 的
String.prototype.trim()会剥掉 U+00A0（以及 U+FEFF、U+1680、U+2000–U+200A、U+202F、U+205F、U+3000），而 git 的read_gitfile只剥 C-locale 的isspace字节。被审代码（沙箱允许其写入.qwen/tmp下任意位置）把工作树的.git改写为gitdir: <U+00A0><repo>/.git/worktrees/review-pr-1，并在挂载之内的<tree>/<U+00A0><repo>/.git/worktrees/review-pr-1放置一个自洽的管理条目（backpointer、commondir、filter.<x>.smudge）。门把 NBSP 剥掉后解析到挂载之外的真实条目并放行；git 却解析到植入条目；被守卫的写入随即在宿主上执行 smudge。 - 解析基准类——
resolve(tree, target)是纯词法的，而 spawn 出的 git 在chdir跟随符号链接之后，按树的物理路径解析相对目标。当审查工作树在挂载之内被替换为一个指向更深攻击者目录的符号链接、且其 gitfile 使用相对目标时，门放行，而 git 落在挂载之内的植入管理条目上。
证据（在本提交上的探针）：入口 1——门返回 null（放行），git rev-parse --absolute-git-dir 解析到植入条目，worktree add 成功，canary 被写出；C-locale trim 补丁后门拒绝。入口 2——门放行，git 解析到的目录位于挂载之内，worktree add 退出码 0，smudge 在宿主上执行；叶子符号链接拒绝补丁后门拒绝、canary 缺失。
入口空间——git 与 JS 模型解析方式不同的每一个字节与路径形态——无法逐角枚举封堵，逐字符补丁不会收敛。请结构性关闭：改为对 git 自己解析出的位置做判定——在树中以 sanitizedGitEnv() spawn git rev-parse --absolute-git-dir（该流程本就在这些树里运行 rev-parse 读取），spawn 失败或 realpath 位于挂载根之内时拒绝。这样门的判定与其后的写入天然针对同一路径，一次性关闭空白/编码类与词法/物理类分歧。并在 untrustedGitfile 测试块中补上这两种形态。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| /** | ||
| * Why a tree's own gitfile cannot be trusted to resolve a host-side git write. | ||
| * | ||
| * Returns a refusal, or null when there is nothing to police. Every host-side |
There was a problem hiding this comment.
[Critical] R5-2: the class invariant this docstring declares — every host-side git command that WRITES resolves the repository through the tree's own .git — is enforced per call site in one consumer only; three host-side writes through mount-resident trees stay ungated, and all three were probed to host execution at this commit.
The gate is wired into test-efficacy.ts's three writes and nowhere else. The remaining entrances all run AFTER the build/test phase that did the planting, against the same rewritable gitfile:
base-tree.ts:268— the rebuildgit worktree add --detachhas no defence at all (and noNO_HOOKS).scratch-tree.ts:587— the rebuildgit worktree add --detachis screened bylocalFilterCommands, whose regex^filter\..*\.(smudge|clean)$does not matchfilter.<x>.process— which executes on the host.fetch-pr.ts:690— the--resumepath runsgit status -C <review worktree>(index-refreshing) with no gate and no-c core.fsmonitor=inert arg; a planted admin entry'score.fsmonitorexecutes on the host.--resumecoexists with the sandbox exactly on the local containerized lane this PR polices, and the fsmonitor fires inside the verygit statusthat collects the ruling evidence — the attack needs no successful resume.
Witness (probes at this commit):
BASE-TREE: {"available": true, "canaryWritten": true} — planted smudge ran on the host, command reported success
SCRATCH-TREE: filter.evil.process → {"canaryWritten": true} (control with plain smudge key: refused, no canary)
RESUME: CANARY CREATED: fsmonitor EXECUTED by git status — FLIP OK: -c core.fsmonitor= prevents execution
A fourth round of per-site fixes would repeat this round's shape; the diff's own comment at the gated worktree add — a maintainer adding a checkout above it would reopen the route — is the tell that the depth is one level too shallow. Apply untrustedGitfile(tree, mountRootFor) before base-tree's and scratch-tree's rebuild worktree add, and at the top of fetch-pr's tryResume (refuse-to-fresh, so cleanStale removes the planted tree); or, better, push the gate down into the shared host-side write path so it cannot be omitted per caller. Give the resume spawns the same inert args (-c core.fsmonitor=, -c core.hooksPath=/dev/null/no-hooks) the restore path passes.
中文说明
[Critical] R5-2：这段文档字符串所宣告的类不变量——「每一个会写入的宿主侧 git 命令都经由树自己的 .git 解析仓库」——目前只在一个消费者里逐调用点执行；仍有三处宿主侧写入透过挂载内的树进行且无门守卫，且三者都在本提交上被探针实测到宿主执行。
门只接入了 test-efficacy.ts 的三处写入。其余入口全部运行在「已完成植入」的构建/测试阶段之后，面对的是同一个可被改写的 gitfile：
base-tree.ts:268——重建用的git worktree add --detach完全没有防御（也没有NO_HOOKS）。scratch-tree.ts:587——重建用的git worktree add --detach有localFilterCommands筛查，但其正则^filter\..*\.(smudge|clean)$匹配不到filter.<x>.process——后者会在宿主上执行。fetch-pr.ts:690——--resume路径运行git status -C <审查工作树>（会刷新索引），既无门，也没有-c core.fsmonitor=惰性参数；植入管理条目的core.fsmonitor会在宿主上执行。--resume与本 PR 所守卫的本地容器化通道恰恰共存，而且 fsmonitor 就在收集裁定证据的那次git status中触发——攻击甚至不需要 resume 成功。
证据（在本提交上的探针）：BASE-TREE——植入的 smudge 在宿主上执行，命令却报告成功；SCRATCH-TREE——filter.evil.process 在宿主上执行（普通 smudge 键的对照组被拒绝、无 canary）；RESUME——git status 期间 fsmonitor 执行、canary 写出，加上 -c core.fsmonitor= 后翻转成功。
再来一轮逐点修复只会重复本轮的形态；diff 自己在被守卫的 worktree add 旁的注释——「日后有人在其上方添加 checkout 就会重新打开这条路线」——正说明深度浅了一层。请把 untrustedGitfile(tree, mountRootFor) 加在 base-tree 与 scratch-tree 的重建 worktree add 之前，以及 fetch-pr 的 tryResume 开头（拒绝并回退为全新流程，让 cleanStale 删除植入的树）；更好的做法是把门下推到共享的宿主侧写入路径，使其不可能被逐调用点遗漏。请给 resume 的 spawn 加上与 restore 路径相同的惰性参数（-c core.fsmonitor=、-c core.hooksPath=/dev/null/no-hooks）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…that were left **Stop re-implementing git's resolver.** The gate parsed `gitdir:` and resolved it in Node while the write that followed resolved the same file in git, and the two disagree in ways that are individually small and collectively unbounded: JS `trim()` strips U+00A0 where git's `read_gitfile` trims only C-locale space, so `gitdir: <NBSP><real entry>` resolved outside the mount here — admitted — and to a planted entry inside it there; and `resolve()` is lexical where a spawned git resolves relative targets against the tree's physical path after chdir. Every divergence admits a pointer whose real referent only git sees, and enumerating them has no last corner. So the resolver that decides is now the resolver that acts: `rev-parse --absolute-git-dir`, run in the tree, with hooks and fsmonitor inert. What stays ours is the only judgment that was ever ours — WHERE the answer lives. The fixtures had to become real repositories for this to mean anything, which is an improvement on its own. **Three more entrances, all probed to host execution.** Round 2 recorded that per-call-site gating does not close the class, and I argued a shared layer would be noise. That was wrong, and the review paid for the answer: base-tree's rebuild had no defence at all; scratch-tree's screen matches `filter.*.smudge|clean` and not `filter.*.process`, which also executes; and `--resume` runs an index-refreshing `git status` through the same pointer, so a planted `core.fsmonitor` fires inside the very command that collects the ruling's evidence — the attack does not need the resume to succeed. All three now ask the same question, and `status` carries an inert fsmonitor besides. **Absence is the caller's question, not this one's.** The first cut refused a tree that does not exist, which is the state `--resume` legitimately asks about — 26 tests, measured. It returns null there now.
…nt' into fix/review-host-state-out-of-mount
|
两条 Critical 都处理了。第二条我在第 2 轮判断错了，先说这个。 R5-1：我在重新实现 git 的解析器这条打中的是根本。我用 JS 解析
所以不再自己解析：做决定的解析器必须就是动手的那个。现在问 代价是夹具必须变成真实仓库，这本身是改进。变异（退回自己解析）现在被 NBSP 那条用例打红；此前它存活。 R5-2：第 2 轮我说共享层是噪音，判断错了你当时记录了「按调用点逐个设门关不掉这个类」，我回说
三处都接上了同一道门， 一处我修出来的回归第一版的门会拒绝尚不存在的树，而那正是 需要如实披露的一处缺口
原因是 会存活的具体变异：删掉 我没有为它编一个证明不了接线的测试。要么单独给 fetch-pr 建一份真实文件系统的测试文件，要么把 review + lease 全部套件 104 文件 5090 通过； 另：远端上有一个以维护者身份推的 main 合并，我把它当基线合并叠加，没有 force-push。它带来的新 workspace 包需要重新装依赖，与本改动无关。 合并之后我把六条护栏逐条重新变异（问 git 的解析、位置判定、整道门、base-tree、scratch-tree、revert），六次全红——合并有可能在测试全绿的情况下拆掉护栏，不重验不算数。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/worktree.ts:186 — [probe] R6/D6-1: the gate's resolution-failure refusal discards git's stderr — every non-attack firing reports one identical generic sentencepackages/cli/src/commands/review/fetch-pr.ts:693 — [review] R6/D6-2: the --resume gate has no refusal test anywhere; deleting or moving it ships greenpackages/cli/src/commands/review/lib/worktree.test.ts:1925 — [review] R6/D6-3: comment claims an NBSP fixture the test does not have — invites deletion of the only plain-shape coveragepackages/cli/src/commands/review/base-tree.test.ts:106 — [review] R6/D6-4: the forged-gitfile fixture is pasted in five places across four files with no shared builderpackages/cli/src/commands/review/base-tree.ts:276 — [review] R6/D6-5: the gate-then-worktree-add sequence is hand-repeated at three entrances; a copy without the gate reopens the route
Convergence: round 6 posted 6 inline comment(s), 6 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/worktree.ts (findings in round 5; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
[Critical] R6-6: fetch-pr's own fresh-path git('worktree', 'add', wt, ref) (fetch-pr.ts:991 — unchanged code outside this delta's hunks, hence no inline anchor) is ungated: it resolves the repository through the LAUNCH directory's .git, which is a rewritable gitfile in the nested/dogfood geometry this PR's own mountRootFor explicitly supports. worktreePath is cwd-relative, lib/git.ts spawns with no cwd override, fetch-pr.ts:868 names process.cwd() the repositoryRoot, and nothing refuses a launch from inside a review temp dir. The diff's own docstring states the class invariant with a universal quantifier ("Every host-side git command that WRITES resolves the repository through the .git of the tree it runs in..."); this is the ungated sixth entrance. Failure scenario: the outer review's containerized build/test phase rewrites <repo>/.qwen/tmp/review-pr-A/.git to a forged admin entry planted under the same mount; every gate this PR adds refuses but never RESTORES the gitfile, so the tampered tree survives; a subsequent review launched from inside that worktree — the geometry mountRootFor's own docstring names, and this pipeline itself runs under — runs the inner fresh fetch with cwd = the tampered worktree, and the add checks files out through the forged entry, executing its planted filter on the host (with the runner's tokens on CI). Witness (three-arm probe at the reviewed commit, reproducing fetch-pr's exact spawn shape): ARM A (ungated, cwd = tampered worktree): {"addExit":0,"out":"HEAD is now at 6b82026 payload"}, markerExists: true — the planted smudge ran ON THE HOST; ARM B (untrustedGitfile(process.cwd()) gate applied): refusal "...admin entry is inside the review temp dir...", addRan: false; ARM C (clean nested control): admitted. Suggested fix: gate the fresh path on the caller directory before the add — const cwdUntrusted = untrustedGitfile(process.cwd(), mountRootFor); if (cwdUntrusted !== null) throw new Error(...) — a no-op outside a mount, refusing exactly the nested case. 中文说明：fetch-pr 自身全新路径的 git('worktree', 'add', wt, ref)（fetch-pr.ts:991——不在本 delta 任何 hunk 内的既有代码，故无法行内锚定）没有门：它通过启动目录的 .git 解析仓库，而本 PR 自己的 mountRootFor 明确支持的嵌套/自举几何下，那是一个可被改写的 gitfile。worktreePath 是相对 cwd 的路径，lib/git.ts 的 spawn 不覆盖 cwd，fetch-pr.ts:868 把 process.cwd() 记为 repositoryRoot，且没有任何东西拒绝从 review 临时目录之内启动。失败场景：外层审查的容器化构建/测试阶段把 <repo>/.qwen/tmp/review-pr-A/.git 改写为同一挂载下植入的伪造管理条目；本 PR 新增的每道门都只拒绝、从不恢复 gitfile，被篡改的树因此存活；随后从该工作树之内启动的审查（mountRootFor 文档字符串命名的几何，也是本流水线自身运行的几何）以 cwd = 被篡改工作树运行内层全新 fetch，add 经由伪造条目检出文件，在宿主上执行植入的过滤器。证据为三臂探针（ARM A 无门：宿主上执行；ARM B 加 untrustedGitfile(process.cwd()) 门：拒绝；ARM C 干净嵌套对照：放行）。修复：在 add 之前对调用方目录做同一位置门（挂载之外是空操作）。
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 6 条行内评论，其中 6 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/worktree.ts（第 5 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R6-6: fetch-pr's own fresh-path git('worktree', 'add', wt, ref) (fetch-pr.ts:991 — unchanged code outside this delta's hunks, hence no inline anchor) is ungated: it resolves the repository through the LAUNCH directory's .git, which is a rewritable gitfile in the nested/dogfood geometry this PR's own mountRootFor explicitly supports. worktreePath is cwd-relative, lib/git.ts spawns with no cwd override, fetch-pr.ts:868 names process.cwd() the repositoryRoot, and nothing refuses a launch from inside a review temp dir. The diff's own docstring states the class invariant with a universal quantifier ("Every host-side git command that WRITES resolves the repository through the .git of the tree it runs in..."); this is the ungated sixth entrance. Failure scenario: the outer review's containerized build/test phase rewrites <repo>/.qwen/tmp/review-pr-A/.git to a forged admin entry planted under the same mount; every gate this PR adds refuses but never RESTORES the gitfile, so the tampered tree survives; a subsequent review launched from inside that worktree — the geometry mountRootFor's own docstring names, and this pipeline itself runs under — runs the inner fresh fetch with cwd = the tampered worktree, and the add checks files out through the forged entry, executing its planted filter on the host (with the runner's tokens on CI). Witness (three-arm probe at the reviewed commit, reproducing fetch-pr's exact spawn shape): ARM A (ungated, cwd = tampered worktree): {"addExit":0,"out":"HEAD is now at 6b82026 payload"}, markerExists: true — the planted smudge ran ON THE HOST; ARM B (untrustedGitfile(process.cwd()) gate applied): refusal "...admin entry is inside the review temp dir...", addRan: false; ARM C (clean nested control): admitted. Suggested fix: gate the fresh path on the caller directory before the add — const cwdUntrusted = untrustedGitfile(process.cwd(), mountRootFor); if (cwdUntrusted !== null) throw new Error(...) — a no-op outside a mount, refusing exactly the nested case. 中文说明：fetch-pr 自身全新路径的 git('worktree', 'add', wt, ref)（fetch-pr.ts:991——不在本 delta 任何 hunk 内的既有代码，故无法行内锚定）没有门：它通过启动目录的 .git 解析仓库，而本 PR 自己的 mountRootFor 明确支持的嵌套/自举几何下，那是一个可被改写的 gitfile。worktreePath 是相对 cwd 的路径，lib/git.ts 的 spawn 不覆盖 cwd，fetch-pr.ts:868 把 process.cwd() 记为 repositoryRoot，且没有任何东西拒绝从 review 临时目录之内启动。失败场景：外层审查的容器化构建/测试阶段把 <repo>/.qwen/tmp/review-pr-A/.git 改写为同一挂载下植入的伪造管理条目；本 PR 新增的每道门都只拒绝、从不恢复 gitfile，被篡改的树因此存活；随后从该工作树之内启动的审查（mountRootFor 文档字符串命名的几何，也是本流水线自身运行的几何）以 cwd = 被篡改工作树运行内层全新 fetch，add 经由伪造条目检出文件，在宿主上执行植入的过滤器。证据为三臂探针（ARM A 无门：宿主上执行；ARM B 加 untrustedGitfile(process.cwd()) 门：拒绝；ARM C 干净嵌套对照：放行）。修复：在 add 之前对调用方目录做同一位置门（挂载之外是空操作）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const untrusted = untrustedGitfile(worktree, mountRootFor); | ||
| if (untrusted !== null) { | ||
| throw new Error(`refusing to create a scratch tree: ${untrusted}`); | ||
| } |
There was a problem hiding this comment.
[Critical] R6-1: This gate guards only the scratch tree's REBUILD path. The REUSE path above it — existsSync(tree) && resetScratchTree(...) (near line 550), whose git checkout --force --detach <headSha> runs near line 354 — resolves the repository through the scratch tree's own rewritable gitfile before this gate is ever consulted, and resetScratchTree's identity checks provably admit exactly the planted shape the adminEntryInsideReviewTmp docstring this diff adds says identity gates "cannot" catch.
Concrete trigger: reviewed code runs sandboxed with .qwen/tmp mounted read-write and rewrites BOTH the scratch tree's and the review worktree's gitfiles to admin entries planted under the same mount whose commondir files name one planted common dir (objects shared via alternates, backpointers coherent, filter.evil.process in the planted config). Any second runScratchTree call for the same label hits resetScratchTree: the localFilterCommands regex ^filter\..*\.(smudge|clean)$ cannot match .process, and every identity question passes — commonOf equality holds because both sides resolve through attacker-rewritten pointers. The checkout then launches the planted filter on the host. A second variant needs no double rewrite: with extensions.worktreeConfig=true in the host repo (set by routine ops such as git sparse-checkout init --cone), a planted config.worktree carrying filter.evil.smudge becomes live through a REAL commondir.
Witness (probe at this commit; flips with the fix below):
BASE, variant B (scratch-gitfile rewrite + worktreeConfig smudge):
FILTER_RAN_ON_HOST: true
report: { available: true, reused: true } <- fully silent
WITH FIX (gate added to resetScratchTree):
FILTER_RAN_ON_HOST: false
Fix: ask the same location question at the top of resetScratchTree, right after the .git existence check — a refusal routes to discard-and-rebuild, whose worktree add this diff already gates:
if (untrustedGitfile(tree, mountRootFor) !== null) return false;and add a regression test planting the double-rewrite shape and asserting reuse is refused and no checkout runs.
中文说明
[Critical] R6-1：这道门只守卫了 scratch tree 的重建（REBUILD）路径。它上方的复用（REUSE）路径——existsSync(tree) && resetScratchTree(...)（约 550 行，其 git checkout --force --detach <headSha> 在约 354 行）——在这道门被查询之前就已通过 scratch tree 自己可被改写的 gitfile 解析仓库，而 resetScratchTree 的身份检查已被证明会放行本 diff 新增的 adminEntryInsideReviewTmp 文档字符串明言身份门「看不见」的那种植入形态。
具体触发：被审代码在 .qwen/tmp 以读写方式挂载的沙箱中运行，把 scratch tree 与审查工作树两个 gitfile 都改写为同一挂载之下植入的管理条目，两者的 commondir 共同指向一个植入的公共目录（对象经 alternates 共享、backpointer 自洽、植入 config 带 filter.evil.process）。同一 label 的第二次 runScratchTree 调用会进入 resetScratchTree：localFilterCommands 的正则 ^filter\..*\.(smudge|clean)$ 匹配不到 .process，且所有身份问题都通过——commonOf 相等成立，因为两侧都经由攻击者改写的指针解析。随后 checkout 在宿主上启动植入的过滤器。另一变体无需双重改写：当宿主仓库带有 extensions.worktreeConfig=true（git sparse-checkout init --cone 等常规操作会设置）时，经真实 commondir 植入的 config.worktree（带 filter.evil.smudge）即会生效。
证据（在本提交上探针；应用下方修复后翻转）：基线变体 B——FILTER_RAN_ON_HOST: true，报告 { available: true, reused: true }（完全无声）；应用修复后——FILTER_RAN_ON_HOST: false。
修复：在 resetScratchTree 开头、.git 存在性检查之后问同一个位置问题（见上方英文部分代码块）——拒绝会落入丢弃并重建路径，其 worktree add 已由本 diff 守卫。并新增回归测试：放置双重改写形态，断言复用被拒绝且未执行 checkout。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const resumeUntrusted = untrustedGitfile(wt, mountRootFor); | ||
| if (resumeUntrusted !== null) { | ||
| throw new Error(`refusing to resume: ${resumeUntrusted}`); | ||
| } |
There was a problem hiding this comment.
[Critical] R6-2: This gate refuses-to-crash instead of the refuse-to-fresh this file's own contract promises. The throw propagates out of runFetchPr — the enclosing catch (~line 1745) rolls back the lease and re-throws — so cleanStale (~line 915) is never reached and the planted tree is never removed. That contradicts the invariant stated near lines 895-897 ("A refused resume falls through to the fresh path and announces why"), the --resume option describe (~1924-1929), docs/users/features/code-review.md:413 ("the flag never fails a run that could start over"), and the round-5 blocker's explicit remediation ("refuse-to-fresh, so cleanStale removes the planted tree").
Concrete trigger: reviewed code in the sandboxed build/test phase rewrites .qwen/tmp/review-pr-N/.git — the exact attack this PR polices. Every subsequent --resume fetch-pr throws refusing to resume and exits non-zero; the lease rolls back, but the planted tree persists, so every retry fails identically — a wedge on that PR on that machine until an operator deletes the tree by hand. The documented fall-through removes the tree via cleanStale and starts a fresh review; the non-resume control proves cleanStale does remove it.
Witness (probe driving the real handler at this commit; flips with the fix):
attempt1 (--resume): threw: refusing to resume: ...admin entry is inside the review temp dir
treeExistsAfterAttempt1: true
attempt2 (--resume): identical throw; treeExistsAfterAttempt2: true <- the wedge
non-resume control: treeExistsAfterNonResume: false <- cleanStale removes it
WITH FIX (return a refusal): falls through to the fresh path; tree removed
Fix: add an untrusted-gitfile case to the ResumeRefusal union (lib/resume.ts) and return { resumed: false, reason: 'untrusted-gitfile', ... } instead of throwing, so control falls through to cleanStale and the fresh path, announcing via the existing "Cannot resume ... starting a fresh review" output.
中文说明
[Critical] R6-2：这道门是「拒绝即崩溃」，而非本文件自身契约所承诺的「拒绝后回退全新流程」。throw 会传出 runFetchPr——外层 catch（约 1745 行）回滚 lease 后重新抛出——因此 cleanStale（约 915 行）永远到不了，植入的树永远不会被移除。这与约 895-897 行陈述的不变量（「被拒绝的 resume 落入全新路径并宣告原因」）、--resume 选项的 describe（约 1924-1929 行）、docs/users/features/code-review.md:413（「该标志绝不会让一个本可重新开始的运行失败」）以及第 5 轮阻断项明示的修复方案（「拒绝后回退全新流程，让 cleanStale 移除植入的树」）相矛盾。
具体触发：被审代码在沙箱化的构建/测试阶段改写 .qwen/tmp/review-pr-N/.git——正是本 PR 所守卫的攻击。此后每一次 --resume fetch-pr 都抛出 refusing to resume 并以非零退出；lease 被回滚，但植入的树仍在，于是每次重试都以同样方式失败——该 PR 在这台机器上被卡死，直到有人手工删除该树。文档承诺的回退路径会经 cleanStale 移除该树并开始全新审查；非 resume 对照实验证明 cleanStale 确实会移除它。
证据（在本提交上驱动真实处理器的探针；应用修复后翻转）：第 1 次 --resume 抛出 refusing to resume，树仍在；第 2 次相同抛出，树仍在（卡死）；非 resume 对照中树被 cleanStale 移除；应用修复后（返回拒绝而非抛出）落入全新路径、树被移除。
修复：在 ResumeRefusal 联合（lib/resume.ts）中新增 untrusted-gitfile 分支，返回 { resumed: false, reason: 'untrusted-gitfile', ... } 而非抛出，使控制流落入 cleanStale 与全新路径，并经现有「无法 resume……开始全新审查」输出宣告。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const r = run(); | ||
| expect(r.available).toBe(false); | ||
| expect(JSON.stringify(r)).toContain('review temp dir'); |
There was a problem hiding this comment.
[Critical] R6-3: This refusal test is deterministically red on the test_windows merge-queue lane. It asserts the gate's refusal through the full run() path, which depends on mountRootFor returning a mount root — but on Windows mountRootFor refuses EVERY absolute path (drive-letter colon: if (real.includes(':')) return null; — "On Windows this refuses EVERY absolute path"), so the gate never speaks there: git worktree add succeeds, r.available is true, no report field contains review temp dir, and the tree exists — so expect(r.available).toBe(false), this toContain('review temp dir'), and the tree-absence assertion below all fail. This is the R3-1 class this PR already paid one Critical for; the twin test added in scratch-tree.test.ts fails identically. Neither new test carries an it.skipIf(process.platform === 'win32') guard (scratch-tree.test.ts guards three OTHER tests at lines 674/794/957 but not its new one; this file has none). The Test (windows-latest, Node 22.x) lane runs under merge_group and reports as skipped on the PR page, so green here is no evidence — the failure ejects the queue entry and stalls every PR batched with it. (At the time of this review the lane IS failing at HEAD — the red it carries is this very pair of tests.)
Witness: not run — the windows-latest lane is unreachable from this Linux runner; the Windows red is traced to the quoted platform branch, and the Linux arm of both tests passed (the gate speaks only when a mount exists).
Fix — the convention R3-1's own fix established in this PR (test-efficacy.integration.test.ts:273):
it.skipIf(process.platform === 'win32')(
'refuses to build through a rewritten review-worktree gitfile',
() => {中文说明
[Critical] R6-3：这条拒绝测试在 test_windows 合并队列通道上必然失败。它经由完整的 run() 路径断言门的拒绝，而这依赖 mountRootFor 返回挂载根——但在 Windows 上 mountRootFor 拒绝每一个绝对路径（盘符冒号：if (real.includes(':')) return null;——「在 Windows 上拒绝每一个绝对路径」），因此那道门在那里从不发声：git worktree add 成功，r.available 为 true，报告中不含 review temp dir，树也存在——于是 expect(r.available).toBe(false)、此处的 toContain('review temp dir') 与下方的树不存在断言全部失败。这正是本 PR 已经为之付出过一条 Critical 的 R3-1 类别；scratch-tree.test.ts 中新增的孪生测试以同样方式失败。两条新测试都没有 it.skipIf(process.platform === 'win32') 守卫（scratch-tree.test.ts 在 674/794/957 行守卫了另外三条测试，唯独没有其新增的那条；本文件一条也没有）。Test (windows-latest, Node 22.x) 通道只在 merge_group 下运行、在 PR 页面显示为已跳过，所以此处的绿灯不构成证据——失败会把队列条目踢出并卡住同批的所有 PR。（本次审查时该通道在 HEAD 上正是失败状态——它携带的红灯正是这一对测试。）
证据：未运行——本 Linux 机器上够不到 windows-latest 通道；Windows 红灯由上方引用的平台分支推得，两条测试的 Linux 臂均已通过（门只在存在挂载时发声）。
修复——沿用 R3-1 修复在本 PR 中确立的惯例（见上方英文部分代码块）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const r = run(); | ||
| expect(JSON.stringify(r)).toContain('review temp dir'); | ||
| expect( | ||
| existsSync(scratchWorktreePath(worktree, 'verify--round-1--abc123')), | ||
| ).toBe(false); |
There was a problem hiding this comment.
[Critical] R6-3 (second location): Same defect as the base-tree sibling. This refusal test asserts review temp dir through the full run() path, but on Windows mountRootFor refuses every absolute path (drive-letter colon), so the gate never speaks there: worktree add succeeds and neither this toContain('review temp dir') assertion nor the tree-absence assertion below it can hold. Deterministically red on the test_windows merge-queue lane — the R3-1 class this PR already paid one Critical for. No it.skipIf(process.platform === 'win32') guard: this file guards three OTHER tests (lines 674/794/957) but not the one this delta adds. The lane runs under merge_group only and shows as skipped on the PR page, so green here is no evidence — the failure ejects the queue entry and stalls every PR batched with it. (The lane IS failing at HEAD — this pair of tests is the red it carries.)
Witness: not run — windows-latest unreachable from this Linux runner; the Windows red is traced to the quoted platform branch; the Linux arm of this test passed.
Fix:
it.skipIf(process.platform === 'win32')(
'refuses to stand one up through a rewritten review-worktree gitfile',
() => {中文说明
[Critical] R6-3（第二处）：与 base-tree 中的孪生测试同一缺陷。本测试经由完整 run() 路径断言 review temp dir，但在 Windows 上 mountRootFor 拒绝每一个绝对路径（盘符冒号），门从不发声：worktree add 成功，此处的 toContain('review temp dir') 与其下方的树不存在断言都不可能成立。在 test_windows 合并队列通道上必然失败——正是本 PR 已付出过一条 Critical 的 R3-1 类别。没有 it.skipIf(process.platform === 'win32') 守卫：本文件守卫了另外三条测试（674/794/957 行），唯独没有本 delta 新增的这条。该通道只在 merge_group 下运行、在 PR 页面显示为已跳过，此处的绿灯不构成证据——失败会把队列条目踢出并卡住同批的所有 PR。（本次审查时该通道在 HEAD 上正是失败状态——这一对测试正是它携带的红灯。）
证据：未运行——本 Linux 机器上够不到 windows-latest；Windows 红灯由引用的平台分支推得；本测试的 Linux 臂已通过。
修复：见上方英文部分代码块。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const resolved = spawnSync( | ||
| 'git', | ||
| [ | ||
| '-c', | ||
| 'core.hooksPath=/dev/null/no-hooks', |
There was a problem hiding this comment.
[Critical] R6-4: Check-then-act TOCTOU at every gate site: the gate validates the gitfile in one spawned git process (this spawn) and the caller then WRITES through it in a second spawn, while the file stays writable between the two. The comment below — "the resolver that decides is the resolver that acts" — holds only when the pointer is stable across the two spawns; nothing enforces that. The pipeline runs concurrent phases against one common dir (this file's own words, ~lines 401-404), and all of .qwen/tmp — every PR's tree — sits inside one mount.
Measured at this commit with a race probe against the real gate and real writes (a 3ms/3ms hold swapper modeling a concurrent writer with RW on the mount):
gated: 180, admitted: 320, escapes: 226, escapeRateOfAdmitted: 0.7063
e.g. iter 7: destPointer: gitdir: .../repo/.qwen/tmp/.evil-repo/worktrees/dest-7
— the planted filter ran ON THE HOST
fix arm (write spawned -C <main repo>, resolution through the trusted common dir),
same attacker: admitted: 247, escapes: 0
Reachability premise, stated for you to rule on: a concurrent writer holding the plant for milliseconds during a host-side gate→write window — verifier shards run concurrently, scratch/base trees are rebuilt while probes run, and the reviewed code is the attacker by this PR's own threat model. If the premise does not hold in the real pipeline, say so with the ordering argument — but the measured window and the route are real either way, and the fix arm proves the structural close.
Fix (proven by the fix arm): resolve host-side writes through the trusted common dir — spawn the write from the main repository (-C) or pin GIT_COMMON_DIR — so no mount-writable gitfile mediates a host-side write. At minimum, state at each gate site that it polices plants LEFT by earlier phases, not a writer concurrent with the gated spawn.
中文说明
[Critical] R6-4：每个门位点上的检查后行动（TOCTOU）：门在一个 spawn 的 git 进程中校验 gitfile（即此处 spawn），调用方随后在第二个 spawn 中经由它执行写入，而该文件在两次 spawn 之间始终可写。下方的注释——「做决定的解析器就是执行写入的解析器」——只有在指针跨两次 spawn 保持稳定的前提下才成立，而没有任何机制保证这一点。流水线本就在对一个公共目录并发运行多个阶段（本文件约 401-404 行自己的话），且整个 .qwen/tmp——每个 PR 的树——都在同一个挂载之内。
在本提交上用真实门与真实写入做竞争探针（3ms/3ms 保持型交换器，模拟对挂载有读写权的并发写者）实测：gated: 180, admitted: 320, escapes: 226，放行写入中 70.6% 执行了植入过滤器（宿主上）；修复臂（写入以 -C <主仓库> spawn、经可信公共目录解析），同一攻击者：admitted: 247, escapes: 0。
可达性前提，留待你裁定：在宿主侧门→写入窗口内把植入保持数毫秒的并发写者——验证分片并发运行、探针运行期间 scratch/base 树在重建、而按本 PR 自己的威胁模型被审代码就是攻击者。若该前提在真实流水线中不成立，请给出时序上的论证——但无论前提如何，实测窗口与路线都是真的，且修复臂已证明结构性关闭有效。
修复（修复臂已证明）：让宿主侧写入经可信公共目录解析——从主仓库 spawn 写入（-C）或钉住 GIT_COMMON_DIR——使任何挂载内可写的 gitfile 都不再经手宿主侧写入。至少，在每个门位点写明：它守卫的是早前阶段留下的植入，而非与被守卫 spawn 并发的写者。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // relative path starting with `..`, which the prefix test read as an escape. | ||
| // Both are places a planted entry can sit, and both are why path containment | ||
| // belongs in one tested helper rather than in each caller's arithmetic. | ||
| return isSubpath(realRoot, real); |
There was a problem hiding this comment.
[Critical] R6-5: The containment verdict compares two paths as case-sensitive strings, so on a case-insensitive (or normalization-insensitive) filesystem — default macOS APFS, a host this pipeline explicitly supports — a planted admin entry spelled with different case or Unicode normalization is judged OUTSIDE the review temp dir while resolving INSIDE it, and the gate admits. rev-parse prints the path exactly as spelled in the gitfile (git never re-canonicalises case), and realpathSync is userspace string code that returns the queried spelling whenever lookup succeeds case-insensitively — it never asks the filesystem for the stored name.
Proven end-to-end at this commit against the real gate functions, on a case-insensitive case-preserving mount:
CONTROL (pointer spelled as stored): refused ("...inside the review temp dir...")
ATTACK (.QWEN/TMP spelling): revParseStatus 0, gate: null (ADMITS)
subsequent gated worktree add: 'HEAD is now at bc96b98 evil'
PWNED created ON THE HOST by the planted smudge
flip (case-fold + NFC before compare): attack refused, control unchanged
path.relative('<repo>/.qwen/tmp', '<repo>/.QWEN/TMP/.evil-git') = '../../.QWEN/TMP/.evil-git' → "outside". Linux's case-sensitive lane is unaffected; this defeats every gate the delta adds on the macOS lane. The NFC/NFD variant rides the identical code path.
| return isSubpath(realRoot, real); | |
| return isSubpath( | |
| realRoot.normalize('NFC').toLowerCase(), | |
| real.normalize('NFC').toLowerCase(), | |
| ); |
(add a refusal test spelling a planted pointer with different case)
中文说明
[Critical] R6-5：包含性判定以区分大小写的字符串比较两条路径，因此在大小写不敏感（或规范化不敏感）的文件系统——默认的 macOS APFS，本流水线明确支持的宿主——上，以不同大小写或 Unicode 规范化拼写的植入管理条目会被判为在 review 临时目录之外、而实际解析在其之内，门于是放行。rev-parse 按 gitfile 中的原拼写输出路径（git 从不重新规范化大小写），而 realpathSync 是用户态字符串代码：只要查找能以大小写不敏感方式成功，它就返回查询所用的拼写——从不向文件系统询问存储的名字。
在本提交上于大小写不敏感、大小写保持的挂载上对真实门函数端到端实测：对照（按存储拼写）被拒绝；攻击（.QWEN/TMP 拼写）——revParseStatus 0、门返回 null（放行），随后被守卫的 worktree add 输出 HEAD is now at bc96b98 evil，植入的 smudge 在宿主上写出 PWNED；翻转（比较前做大小写折叠 + NFC）——攻击被拒绝，对照不变。
path.relative('<repo>/.qwen/tmp', '<repo>/.QWEN/TMP/.evil-git') = '../../.QWEN/TMP/.evil-git' → 「在外」。Linux 区分大小写的通道不受影响；此缺陷在 macOS 通道上使本 delta 新增的每道门失效。NFC/NFD 变体走完全相同的代码路径。
修复见上方 suggestion 代码块（比较前对两侧做 NFC 规范化与大小写折叠），并新增一条以不同大小写拼写植入指针的拒绝测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…r than papered over Case, first: the verdict is a string comparison, and Node's JS `realpathSync` hands back the spelling it was asked for whenever the lookup succeeded — so on a case-insensitive filesystem (default macOS APFS, a host this pipeline supports) a planted entry spelled with different case resolves INSIDE the mount and compares OUTSIDE it. `realpathSync.native` asks the operating system for the stored name, so both sides arrive spelled as the filesystem holds them. Two more entrances. scratch-tree's REUSE path reaches a `checkout --force` of its own through the scratch tree's pointer, and fetch-pr's FRESH path checks out through the review worktree's. The reuse one is not a refusal, though: an unusable leftover is exactly what the rebuild exists for, and refusing there turned "rebuild over it" into "fail the command" — three tests said so. And `--resume` refused to CRASH where this file's own contract, the option's describe and the docs all promise refuse-to-FRESH. Throwing propagated out of `runFetchPr`, so `cleanStale` — the thing that removes the planted tree — was never reached. It returns a refusal reason now and the fresh path sweeps. The two new refusal tests were red on Windows, for the third time in this PR and for the same reason: `mountRootFor` refuses every absolute Windows path, so containment is unavailable there by design and the gate never speaks. Both gated off win32 like their siblings. Two limits I could not close, stated where they are rather than left looking proven. The `--resume` gate's wiring is pinned by no test: `fetch-pr.test.ts` mocks `node:fs` without `existsSync`, so the tree never exists there and the gate is inert — deleting it ships green. And the reuse-path gate is defence in depth rather than a demonstrated check: with both trees pointed at one planted common dir and a `filter.*.process` the existing screen misses, removing the gate still executes nothing, because the reset declines that tree for its own reasons first.
|
修掉的大小写（R6-6） —— 这条最漂亮。判定是字符串比较，而 Node 的 JS 两个漏掉的入口（R6-1 / R6-7） —— scratch-tree 的复用路径有自己的 refuse-to-fresh（R6-2） —— 你完全对，而且这条我该自己看出来：抛出会穿出 Windows（R6-3 / R6-4） —— 同一个坑我在本 PR 里第三次踩：前两轮我把旧测试都门控了，这轮新加的两条又没有。已按兄弟套件的形式门控。 我修出来的一个回归复用门的第一版是硬拒绝，把「残留树不可用就重建」变成了「命令失败」——实测打红三条测试。改成「不可信即不可复用」，落到重建路径（重建本身已设门，且会连植入一起丢掉）。 两条我证明不了的1. 2. 复用门是纵深防御，不是我演示过的承重检查。 我按你描述的完整形态搭了夹具：两棵树都改写、指向同一个植入共同库、用 所以门我留着（便宜、意图正确），但测试改名成了它实际钉住的东西，并在注释里写明这一点。如果你的探针确实看到 reset 走到了 checkout，请把那个形态给我——我大概漏了一个条件，而不是想反驳你。 review + lease 全部套件 104 文件 5139 通过。tsc / lint / prettier 干净。 |
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): src/services/review-worktree-lease.test.ts — no such file or directory.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/worktree.ts:239 — [review] D7-1: the R6-6 fix ships with no case-variant refusal test — a revert to JS realpathSync resurrects the APFS bypass greenpackages/cli/src/commands/review/fetch-pr.ts:1005 — [review] D7-2: the fresh-path gate has no test; deleting or moving it ships greenpackages/cli/src/commands/review/fetch-pr.ts:700 — [review] D7-3: the refusal discards untrustedGitfile's reason string — an escape attempt and an environment fault announce identicallypackages/cli/src/commands/review/fetch-pr.ts:1001 — [review] D7-4: the fresh-path gate's rationale comment describes a scenario cleanStale does not admitpackages/cli/src/commands/review/scratch-tree.test.ts:137 — [probe] D7-5: the both-trees fixture's gitdir backpointer uses the wrong format — with git's real bare-path format the reuse gate is proven load-bearing, and the HONEST LIMIT concl…packages/cli/src/commands/review/fetch-pr.ts:710 — [probe] D7-6: the new comment claims -c core.fsmonitor= makes the status execution-proof; hooks and content filters survive that flag
Convergence: round 7 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 6 (6 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/worktree.ts (findings in round 6; 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] R7-2: The residue probe executes planted content filters through the poisoned review-worktree pointer BEFORE any gate this diff adds can speak. runScratchTree calls worktreeResidue(worktree, RESIDUE_PATH_CAP, args.fetchedSha) at scratch-tree.ts:517 — before the reuse gate (562) and the rebuild gate (609) this diff adds. worktreeResidue's probe spawns git --git-dir=<admin entry> --work-tree=<worktree> -c core.fsmonitor= status --porcelain --untracked-files=all -z, resolving the admin entry through the review worktree's rewritable gitfile. With a coherent plant — bare-path gitdir backpointer (the format git itself writes) and filter.evil.process + * filter=evil in the common dir — worktreeResidue's identity checks pass (the PR's own description states why identity gates cannot see this attack: the writer chooses both the gitfile's target and that target's backpointer, so the round-trip agrees with itself), and the status's index refresh executes the planted clean filter ON THE HOST. fsmonitor is emptied there; content filters are not — and no fixed -c set can disarm attacker-named filter keys anyway, so this call needs the same location gate the writes get. The diff's own fetch-pr comment shows the author understood status can execute planted code ("status REFRESHES THE INDEX ... a planted core.fsmonitor executes on the host") and hardened the resume-path status for fsmonitor; the residue status stays exposed to content filters. Witness (round-2 verifier probe, both-trees fixture with bare-path backpointers, ALL diff gates intact): 5/5 vitest runs created the host canary; spawn-log transition: CREATED BY: git --git-dir=…/.evil-wt --work-tree=…/review-pr-1 -c core.fsmonitor= status --porcelain --untracked-files=all -z (canaryBefore false → true); baseline (fixture as committed, prefixed backpointers) stays green only because worktreeResidue's backpointer round-trip declines the malformed entry before its status runs — so the committed test masks this route too. The probe recorded sharedTreeUnmeasured: "git status exited 128" — the filter wrote the canary then died without speaking the process protocol, so the pipeline only notes "residue unmeasured": host execution happens silently. This violates the diff's own stated test property ('runs nothing on the host when BOTH trees point at a planted repository') with every gate intact. Suggested fix: run untrustedGitfile(worktree, mountRootFor) before worktreeResidue at scratch-tree.ts:517 (and at the other worktreeResidue call sites that read a mount-resident tree) and treat a refusal as unmeasured-with-reason. Not anchorable inline: scratch-tree.ts:517 sits outside this diff's hunks.
中文说明
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 2 条行内评论，其中 1 条是首次提出；上一轮发布了 6 条（其中 6 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/worktree.ts（第 6 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R7-2: The residue probe executes planted content filters through the poisoned review-worktree pointer BEFORE any gate this diff adds can speak. runScratchTree calls worktreeResidue(worktree, RESIDUE_PATH_CAP, args.fetchedSha) at scratch-tree.ts:517 — before the reuse gate (562) and the rebuild gate (609) this diff adds. worktreeResidue's probe spawns git --git-dir=<admin entry> --work-tree=<worktree> -c core.fsmonitor= status --porcelain --untracked-files=all -z, resolving the admin entry through the review worktree's rewritable gitfile. With a coherent plant — bare-path gitdir backpointer (the format git itself writes) and filter.evil.process + * filter=evil in the common dir — worktreeResidue's identity checks pass (the PR's own description states why identity gates cannot see this attack: the writer chooses both the gitfile's target and that target's backpointer, so the round-trip agrees with itself), and the status's index refresh executes the planted clean filter ON THE HOST. fsmonitor is emptied there; content filters are not — and no fixed -c set can disarm attacker-named filter keys anyway, so this call needs the same location gate the writes get. The diff's own fetch-pr comment shows the author understood status can execute planted code ("status REFRESHES THE INDEX ... a planted core.fsmonitor executes on the host") and hardened the resume-path status for fsmonitor; the residue status stays exposed to content filters. Witness (round-2 verifier probe, both-trees fixture with bare-path backpointers, ALL diff gates intact): 5/5 vitest runs created the host canary; spawn-log transition: CREATED BY: git --git-dir=…/.evil-wt --work-tree=…/review-pr-1 -c core.fsmonitor= status --porcelain --untracked-files=all -z (canaryBefore false → true); baseline (fixture as committed, prefixed backpointers) stays green only because worktreeResidue's backpointer round-trip declines the malformed entry before its status runs — so the committed test masks this route too. The probe recorded sharedTreeUnmeasured: "git status exited 128" — the filter wrote the canary then died without speaking the process protocol, so the pipeline only notes "residue unmeasured": host execution happens silently. This violates the diff's own stated test property ('runs nothing on the host when BOTH trees point at a planted repository') with every gate intact. Suggested fix: run untrustedGitfile(worktree, mountRootFor) before worktreeResidue at scratch-tree.ts:517 (and at the other worktreeResidue call sites that read a mount-resident tree) and treat a refusal as unmeasured-with-reason. Not anchorable inline: scratch-tree.ts:517 sits outside this diff's hunks.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const freshUntrusted = untrustedGitfile(wt, mountRootFor); | ||
| if (freshUntrusted !== null) { |
There was a problem hiding this comment.
[Critical] R6-7: Round 6's blocker is still open — this fix gates the wrong pointer. cleanStale (line 925) runs unconditionally before step 4 and removes whatever tree stands at wt, and untrustedGitfile returns null for a nonexistent tree, so this gate is a no-op on every reachable fresh path (the only exception is the corner where releaseWorktree itself declined, which cleanStale announces on stderr). Meanwhile git('worktree', 'add', wt, ref) inherits process.cwd() — lib/git.ts sets no cwd — so the repository resolves through the LAUNCH directory's .git, exactly the pointer R6-7's demonstrated attack rewrites. In the nested/dogfood geometry mountRootFor's own docstring names, the outer review's containerized build/test phase rewrites that gitfile to a forged admin entry under the same mount; the inner fresh fetch passes this gate and worktree add checks files out through the forged entry — the planted filter executes on the host, with the runner's tokens on CI. Round 6's witness measured exactly this at the pre-fix commit, and no process.cwd() gate exists anywhere at HEAD. The comment above ("a pointer rewritten in THIS run's build phase is still here") names a state fetch-pr cannot reach: it runs no build phase, and any earlier-planted tree is removed by cleanStale above.
Witness (probe at e89cdd0 replicating fetch-pr's exact fresh-path sequence; flips both ways):
BASE/HEAD arm: {gateOnWt: null, addExit: 0, canaryExists: true, innerAdminDeposited: true}
— the planted smudge ran ON THE HOST
fix arm (untrustedGitfile(process.cwd(), mountRootFor) before the add):
{refusedByFix: true, addExit: null, canaryExists: false}
| const freshUntrusted = untrustedGitfile(wt, mountRootFor); | |
| if (freshUntrusted !== null) { | |
| const cwdUntrusted = untrustedGitfile(process.cwd(), mountRootFor); | |
| if (cwdUntrusted !== null) { | |
| throw new Error( | |
| `refusing to create a review worktree from an untrusted launch directory: ${cwdUntrusted}`, | |
| ); | |
| } | |
| const freshUntrusted = untrustedGitfile(wt, mountRootFor); | |
| if (freshUntrusted !== null) { |
中文说明
[Critical] R6-7：第 6 轮的阻断项仍然开放——这个修复门错了对象。cleanStale（925 行）在 step 4 之前无条件运行，移除 wt 处的任何树，而 untrustedGitfile 对不存在的树返回 null，因此在每条可达的全新路径上这道门都是空操作（唯一的例外是 releaseWorktree 自身拒绝释放的角落，cleanStale 会在 stderr 上通告）。同时 git('worktree', 'add', wt, ref) 继承 process.cwd()——lib/git.ts 不设置 cwd——仓库经由启动目录的 .git 解析，正是 R6-7 演示的攻击所改写的那个指针。在 mountRootFor 自身文档字符串命名的嵌套/自举几何里，外层审查的容器化构建/测试阶段把那个 gitfile 改写为同一挂载下伪造的管理条目；内层全新 fetch 通过这道门，worktree add 经由伪造条目检出文件——植入的过滤器在宿主上执行，在 CI 上带着 runner 令牌。第 6 轮的证人在修复前的提交上实测了完整链路，而 HEAD 上任何位置都不存在 process.cwd() 门。上方注释（「本次运行的构建阶段改写的指针仍在这里」）指向一个 fetch-pr 够不到的状态：它不运行构建阶段，且任何更早植入的树都会被上面的 cleanStale 清掉。
证据（在 e89cdd0 上复现 fetch-pr 全新路径确切时序的探针；双向翻转）：BASE/HEAD 臂 {gateOnWt: null, addExit: 0, canaryExists: true, innerAdminDeposited: true}——植入的 smudge 在宿主上执行；修复臂（在 add 之前加 untrustedGitfile(process.cwd(), mountRootFor)）{refusedByFix: true, addExit: null, canaryExists: false}。修复建议：在 add 之前对启动目录做同一位置门（挂载之外是空操作）；wt 门可以保留以覆盖 releaseWorktree 拒绝的角落，但它关不上这个入口。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const target = resolved.stdout.trim(); | ||
| if (adminEntryInsideReviewTmp(target, mountRoot, tree)) { |
There was a problem hiding this comment.
[Critical] R7-1: JS .trim() on git's stdout re-opens the exact divergence the ASK GIT comment above names as why the first cut was wrong. Git's read_gitfile trims only C-locale whitespace; JS trim() also strips U+00A0, U+FEFF and the U+2000 block. So a gitfile target whose directory name ENDS in one of those is resolved by git with the character kept, but judged here with it removed — the resolver that decides is no longer the resolver that acts. Concretely: reviewed code plants a coherent admin entry under the mount whose directory name ends in U+00A0 (carrying filter.<x>.process + * filter=evil), plants the same name minus the NBSP as a symlink to any path outside the mount, and rewrites the worktree's gitfile to the NBSP entry. rev-parse prints the NBSP-suffixed path; .trim() strips it; the judgment lands on the twin symlink, whose realpath resolves outside the mount — admitted at every gate site this diff adds. The gated write then re-resolves the gitfile itself and checks out through the NBSP entry, executing the planted filter on the host. On Linux any non-/ byte is a legal filename character and U+00A0 survives macOS normalization, so both supported hosts admit the shape. This is a new entrance of the R5-1 class the round-5 fix closed (Node-side gitfile parsing); the stdout boundary survived.
Witness (probe driving the REAL untrustedGitfile at e89cdd0; flips with the one-line fix):
armA (NBSP entry + twin symlink): gitAnswerHexTail "…c2a0 0a" (git keeps the NBSP),
gate: null (admitted), addExit: 128, canaryExists: true — filter ran ON THE HOST
armB (same plant, no NBSP): refused
armC (NBSP, no twin): refused (fail-closed — the twin is one symlink in a dir the attacker owns)
AFTER one-line fix (trim() → replace(/\n$/, '')): armA refused, no canary
| const target = resolved.stdout.trim(); | |
| if (adminEntryInsideReviewTmp(target, mountRoot, tree)) { | |
| const target = resolved.stdout.replace(/\n$/, ''); | |
| if (adminEntryInsideReviewTmp(target, mountRoot, tree)) { |
The minimal fix above was flip-tested; rev-parse --path-format=NUL --absolute-git-dir (git ≥ 2.31) with stdout.split('\0')[0] is the more robust variant — your call.
中文说明
[Critical] R7-1：对 git 的 stdout 做 JS .trim()，重新打开了上方 ASK GIT 注释指名道姓、正是第一版被废弃原因的那个分歧。git 的 read_gitfile 只剥 C-locale 空白；JS trim() 还会剥掉 U+00A0、U+FEFF 与 U+2000 区段。于是目录名以这些字符之一结尾的 gitfile 目标，git 解析时保留该字符，这里判定时却把它剥掉——做判决的解析器不再是动手的解析器。具体形态：被审代码在挂载下植入一个目录名以 U+00A0 结尾的自洽管理条目（携带 filter.<x>.process 与 * filter=evil），把去掉 NBSP 的同名路径植入为指向挂载外任意路径的符号链接，并把工作树的 gitfile 改写指向 NBSP 条目。rev-parse 打印带 NBSP 的路径；.trim() 剥掉它；判定落在孪生符号链接上，其 realpath 解析到挂载之外——本 diff 新增的每一道门都放行。随后被门授权的写入自行重新解析 gitfile，经由 NBSP 条目检出，在宿主上执行植入的过滤器。在 Linux 上任何非 / 字节都是合法文件名字符，U+00A0 也能通过 macOS 规范化，两个受支持宿主都容纳这个形态。这是第 5 轮修复关掉的 R5-1 类别（Node 侧解析 gitfile）的一个新入口：stdout 边界幸存了下来。
证据（在 e89cdd0 上驱动真实 untrustedGitfile 的探针；单行修复后翻转）：armA（NBSP 条目 + 孪生符号链接）：gitAnswerHexTail "…c2a0 0a"（git 保留 NBSP），gate: null（放行），canaryExists: true——过滤器在宿主上执行；armB（同植入、无 NBSP）：拒绝；armC（NBSP、无孪生）：拒绝（失败即关闭——孪生只是攻击者已拥有的目录里的一个符号链接）。单行修复（trim() → replace(/\n$/, '')）后 armA 被拒绝、无 canary。上方最小修复已做翻转验证；rev-parse --path-format=NUL --absolute-git-dir（git ≥ 2.31）配合 stdout.split('\0')[0] 是更稳的变体——由你选择。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…actually used Two blockers, both inside the previous round's own fixes. `.trim()` on `rev-parse`'s stdout re-opened the exact divergence the paragraph above it closes. It strips U+00A0, U+FEFF and the U+2000 block, so an admin entry whose directory NAME ends in one is resolved by git with the character and judged here without it — and a twin of that name minus the character, symlinked outside the mount, is where the judgment then lands. Only the terminator `rev-parse` adds may be removed. And the fresh-path gate checked the wrong pointer. `git()` sets no cwd, so `worktree add` resolves the repository from `process.cwd()` — while the gate asked about `wt`, which `cleanStale` has just removed, and a tree that does not exist has no pointer to distrust. It was a no-op on every reachable path. It asks about the launch directory now, which is the one the nested geometry `mountRootFor` documents lets an outer review's containerized phase rewrite.
|
R7-1：我用来关闭那个分歧的修复，自己又把它打开了
上一轮我写下「做决定的解析器必须就是动手的那个」，然后在下一行把 git 的答案编辑了。现在只剥 R6-7：我修的是错的指针
关于这个 PR 的走向，我想说一句到这一轮为止， 同时新 Critical 的轨迹是 6 → 2 → 1 → 1 → 2 → 7 → 1，其中相当一部分是前一轮修复的下游后果。 我不认为这说明这些发现不对（它们都对，而且都带实证）。我认为它说明**「按入口逐个设门」这条路本身在这个问题上不收敛**：入口在增加，而每道门都要独立地把 git 的语义猜对。 lease 那一半不同——它是一次减法，把宿主可信状态挪出可写面，已经稳定了好几轮。 所以我把选择摆给维护者：是继续在本 PR 里补门，还是把 gitfile 那一半拆出去、先落地 lease 迁移。我不替这个决定做主，但如果继续，我会继续按同样的标准跟。 review + lease 全部套件 104 文件 5140 通过。tsc / lint / prettier 干净。 |
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 (merge_group only) and the Windows arm did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 6a": none — no check was cut short..
Not reviewed: reverse audit — stopped before round 8 by the review time budget.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/worktree.ts:186 — [probe] D8-1: two fail-closed refusal branches of untrustedGitfile have zero test coverage; a fail-open flip ships greenpackages/cli/src/commands/review/fetch-pr.ts:1010 — [review] D8-2: the R6-7 fix (launch-dir gate) ships with no regression test; no test can fire either fetch-pr gate; a revert to wt ships greenpackages/cli/src/commands/review/fetch-pr.ts:700 — [review] D8-3: the worktree-untrusted resume refusal and its refuse-to-fresh fall-through have zero test coveragepackages/cli/src/commands/review/lib/worktree.ts:185 — [review] D8-4: the resolution-failure refusal discards git's stderr; every environmental fault announces the same sentence as an attack (echoes round-6 deferred D6-1, unchanged code)packages/cli/src/commands/review/lib/worktree.test.ts:1883 — [review] D8-5: the new itWhereContainmentExists skip carries no reason comment; the siblings' stated reason provably cannot apply to this suitepackages/cli/src/commands/review/scratch-tree.test.ts:94 — [probe] D8-6: the reuse gate's only test plants a malformed (prefixed) backpointer; removing the gate leaves the suite green 48/48 (echoes round-7 deferred D7-5)
Convergence: round 8 posted 7 inline comment(s), 7 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/lib/worktree.ts (findings in round 7; 3 more now); packages/cli/src/commands/review/fetch-pr.ts (findings in round 6; 2 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] R7-2: The residue probe executes planted content filters through the poisoned review-worktree pointer BEFORE any gate this diff adds can speak. runScratchTree calls worktreeResidue(worktree, RESIDUE_PATH_CAP, args.fetchedSha) at scratch-tree.ts:517 — before the reuse gate (~562) and the rebuild gate (609). With a coherent plant (bare-path gitdir backpointer — the format git itself writes — filter.evil.process + * filter=evil in the common dir, racy-clean mtimes), worktreeResidue's identity checks pass (the writer chose both halves of the round-trip) and the status's index refresh executes the planted filter on the host; the pipeline records only sharedTreeUnmeasured: 'git status exited 128' — silent host execution with every gate intact. The increment e89cdd0..36fde42 does not touch this path (scratch-tree.ts byte-identical). Round-8 verifier probe: 3/3 runs through the real worktreeResidue created the host canary; the committed property test masks the route — its fixture writes a 'gitdir: '-prefixed backpointer git never writes, so the round-trip declines before status runs. A second ungated residue call site reading a mount-resident tree exists at agent-prompt.ts:1376. Fix: run untrustedGitfile(worktree, mountRootFor) before worktreeResidue at scratch-tree.ts:517 and at agent-prompt.ts:1376, treating a refusal as unmeasured-with-reason. Not anchorable inline: scratch-tree.ts:517 sits outside this round's hunks.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 6a"：none — no check was cut short.。
未审查：反向审计——评审时间预算不足，未能开始第 8 轮。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 8 轮发布了 7 条行内评论，其中 7 条是首次提出；上一轮发布了 2 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/worktree.ts（第 7 轮已出过发现，本轮又有 3 条）；packages/cli/src/commands/review/fetch-pr.ts（第 6 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R7-2: The residue probe executes planted content filters through the poisoned review-worktree pointer BEFORE any gate this diff adds can speak. runScratchTree calls worktreeResidue(worktree, RESIDUE_PATH_CAP, args.fetchedSha) at scratch-tree.ts:517 — before the reuse gate (~562) and the rebuild gate (609). With a coherent plant (bare-path gitdir backpointer — the format git itself writes — filter.evil.process + * filter=evil in the common dir, racy-clean mtimes), worktreeResidue's identity checks pass (the writer chose both halves of the round-trip) and the status's index refresh executes the planted filter on the host; the pipeline records only sharedTreeUnmeasured: 'git status exited 128' — silent host execution with every gate intact. The increment e89cdd0..36fde42 does not touch this path (scratch-tree.ts byte-identical). Round-8 verifier probe: 3/3 runs through the real worktreeResidue created the host canary; the committed property test masks the route — its fixture writes a 'gitdir: '-prefixed backpointer git never writes, so the round-trip declines before status runs. A second ungated residue call site reading a mount-resident tree exists at agent-prompt.ts:1376. Fix: run untrustedGitfile(worktree, mountRootFor) before worktreeResidue at scratch-tree.ts:517 and at agent-prompt.ts:1376, treating a refusal as unmeasured-with-reason. Not anchorable inline: scratch-tree.ts:517 sits outside this round's hunks.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const freshUntrusted = untrustedGitfile(process.cwd(), mountRootFor); | ||
| if (freshUntrusted !== null) { | ||
| throw new Error( | ||
| `refusing to create a review worktree: ${freshUntrusted}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Critical] R8-1: The new launch-directory gate refuses worktree creation whenever the cwd is inside a review temp dir and lacks its own .git — breaking 77 previously-green tests in fetch-pr.test.ts (measured net-new against the merge base). untrustedGitfile(process.cwd(), …) lstats <cwd>/.git, but any subdirectory of a checkout — e.g. packages/cli under .qwen/tmp/review-pr-9983, the geometry this unit suite and the pipeline's own nested/dogfood reviews run in — has no .git there (only the repo root does). The gate then returns "has no .git to resolve" and fetch-pr aborts before git worktree add — which resolves the repository by walking up parents and would succeed — ever runs. Witness: at HEAD 77 of 156 tests in fetch-pr.test.ts fail with the identical Failed to create worktree … refusing to create a review worktree: …/packages/cli has no .git to resolve; test-delta vs merge base a6d30ebc: netNew = fetch-pr.test.ts (fails only on the PR side). Fix: resolve the containing repository's git dir the way worktree add does — gate the result of git rev-parse --absolute-git-dir (or the toplevel's .git) from cwd — rather than requiring cwd/.git itself to exist.
中文说明
[Critical] R8-1：新的启动目录门在 cwd 位于 review 临时目录之内且自身没有 .git 时拒绝创建——打红了 fetch-pr.test.ts 中 77 条原本通过的测试（相对合并基线实测为净新增失败）。untrustedGitfile(process.cwd(), …) 对 <cwd>/.git 做 lstat，但检出的任何子目录——例如 .qwen/tmp/review-pr-9983 之下的 packages/cli，正是本单测套件与流水线自身嵌套/自举审查运行的几何——在那里都没有 .git（只有仓库根有）。于是门返回 "has no .git to resolve"，fetch-pr 在 git worktree add（它会向上走父目录解析仓库、本可成功）运行之前就中止。证据：HEAD 上 fetch-pr.test.ts 156 条测试中 77 条以完全相同的 Failed to create worktree … refusing to create a review worktree: …/packages/cli has no .git to resolve 失败；对合并基线 a6d30ebc 的 test-delta：netNew = fetch-pr.test.ts（仅在 PR 侧失败）。修复：按 worktree add 的方式解析所属仓库的 git 目录——对 cwd 取 git rev-parse --absolute-git-dir（或 toplevel 的 .git）做门——而不是要求 cwd/.git 本身存在。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const target = resolved.stdout.replace(/\r?\n$/, ''); | ||
| if (adminEntryInsideReviewTmp(target, mountRoot, tree)) { |
There was a problem hiding this comment.
[Critical] R8-2: The "take git's answer unedited" fix still edits git's answer upstream of the strip: spawnSync(..., { encoding: 'utf8' }) at line 184 lossily decodes git's byte-exact stdout, so an admin-entry path containing an invalid UTF-8 byte is judged at the U+FFFD spelling while git acts on the original bytes — two different filesystem objects, and the attacker controls both. Failure scenario: reviewed code rewrites a pipeline tree's gitfile to gitdir: <mount>/ent<0xFF>ry, plants that entry inside the mount (fake common dir with filter.evil.smudge in config and * filter=evil in info/attributes), and creates a decoy named ent<U+FFFD>ry (bytes EF BF BD) symlinked outside the mount. Node's utf8 decode turns 0xFF into U+FFFD, realpathSync resolves the decoy, the gate judges "outside" and admits; the subsequent host git checkout --force through the same gitfile resolves the raw-byte entry and executes the planted smudge — bypassing every gate call site in this diff. Witness (probe at this commit, flips with the fix): git emitted the raw 0xFF path byte-exact (exit 0); decoded spelling judged inside mount: false, GATE VERDICT: ADMIT; check-attr showed the planted filter; SMUDGE CANARY CREATED: true. With encoding: 'buffer' + lossless round-trip refusal the probe flips to "git's answer is not lossless UTF-8 — refusing", no checkout. Fix: spawn with encoding: 'buffer', strip the trailing terminator from the Buffer, and refuse unless the bytes round-trip UTF-8 losslessly (containment check on bytes). A lossy decode must fail closed, exactly like the unresolvable case.
中文说明
[Critical] R8-2：「原样接受 git 的答案」的修复在剥离终止符之前仍然编辑了 git 的答案：第 184 行的 spawnSync(..., { encoding: 'utf8' }) 有损解码 git 字节精确的 stdout，因此包含非法 UTF-8 字节的管理条目路径会以 U+FFFD 拼写被判定，而 git 按原始字节行事——这是两个不同的文件系统对象，且攻击者两者都能控制。失败场景：被审代码把流水线树的 gitfile 改写为 gitdir: <mount>/ent<0xFF>ry，在挂载内植入该条目（伪造 common dir，config 带 filter.evil.smudge、info/attributes 带 * filter=evil），并创建一个名为 ent<U+FFFD>ry（字节 EF BF BD）、符号链接到挂载之外的诱饵。Node 的 utf8 解码把 0xFF 变成 U+FFFD，realpathSync 解析到诱饵，门判定「在外」而放行；随后宿主上经同一 gitfile 的 git checkout --force 按原始字节条目解析并执行植入的 smudge——绕过本 diff 的所有门。证据（在本提交上的探针，应用修复后翻转）：git 字节精确地输出了含 0xFF 的原始路径（退出码 0）；解码拼写被判 inside mount: false、GATE VERDICT: ADMIT；check-attr 显示植入的过滤器；SMUDGE CANARY CREATED: true。改用 encoding: 'buffer' + 无损往返拒绝后，探针翻转为 "git's answer is not lossless UTF-8 — refusing"，不再检出。修复：以 encoding: 'buffer' spawn，从 Buffer 上剥离终止符，字节不能无损往返 UTF-8 即拒绝（在字节上做包含判断）。有损解码必须失败即关闭，与不可解析的情形完全一致。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const freshUntrusted = untrustedGitfile(process.cwd(), mountRootFor); | ||
| if (freshUntrusted !== null) { | ||
| throw new Error( |
There was a problem hiding this comment.
[Critical] R8-3: The gate this round moved onto the pointer actually used sits at step 4, but step 2's git fetch (line 931) already resolves the repository through that same launch-directory pointer and is a config-driven execution surface — in the very nested geometry this comment invokes, the route executes before the gate is reached. Failure scenario: the outer review's containerized phase rewrites the outer worktree's gitfile to a planted admin entry whose common-dir config sets [credential] helper = !<cmd> and [remote "origin"] url = <server answering 401>. The inner fresh lane runs cleanStale (non-materialising) and then git fetch origin <refspec> with cwd inherited from the poisoned launch directory — before this gate — and git invokes the planted credential helper: arbitrary command execution on the host. The repository-level core.sshCommand form is not scrubbed by sanitizedGitEnv either (the env-var form is; the config form lives in the planted repo). Witness (probe at this commit, GIT_TERMINAL_PROMPT=0 as gitOpts sets): the fetch hit the 401 URL — fatal: could not read Username … terminal prompts disabled, exit 128 — but CREDENTIAL HELPER RAN: /CRED-RAN exists; the helper executed before fetch exited. Fix: hoist the untrustedGitfile(process.cwd(), mountRootFor) check above step 1/2 — before the first command that resolves through the launch directory with execution capability; the read-only commands (remote get-url, rev-parse, worktree remove/unlock/prune, branch -D) may stay ahead of it.
中文说明
[Critical] R8-3：本轮把门移到真正被使用的指针上，但它位于 step 4；而 step 2 的 git fetch（931 行）早已通过同一个启动目录指针解析仓库，且是配置驱动的执行面——就在本注释所引用的嵌套几何里，该路径在到达门之前就已执行。失败场景：外层审查的容器化阶段把外层工作树的 gitfile 改写为植入的管理条目，其 common dir 的 config 设置 [credential] helper = !<cmd> 与 [remote "origin"] url = <返回 401 的服务器>。内层全新流程先运行 cleanStale（不具现任何文件），随后以继承自被毒化启动目录的 cwd 运行 git fetch origin <refspec>——在本门之前——git 调用植入的 credential helper：宿主任意命令执行。仓库级的 core.sshCommand 同样不被 sanitizedGitEnv 清理（环境变量形式会被；配置形式就在植入的仓库里）。证据（本提交上的探针，GIT_TERMINAL_PROMPT=0 与 gitOpts 一致）：fetch 命中 401——fatal: could not read Username … terminal prompts disabled，退出码 128——但 CREDENTIAL HELPER RAN: /CRED-RAN exists；helper 在 fetch 退出前已执行。修复：把 untrustedGitfile(process.cwd(), mountRootFor) 提到 step 1/2 之上——置于第一个「通过启动目录解析且具备执行能力」的命令之前；只读命令（remote get-url、rev-parse、worktree remove/unlock/prune、branch -D）可以留在门前。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const root = mountRoot(tree); | ||
| if (root === null) return false; |
There was a problem hiding this comment.
[Critical] R8-4: The containment ruling judges the admin entry only against the INNERMOST .qwen/tmp containing the tree, but in the nested-review geometry mountRootFor itself documents, a tree's gitfile is writable through EVERY ancestor review temp dir's sandbox mount — an entry planted in an ancestor temp dir is admitted. Failure scenario: outer worktree W = P/.qwen/tmp/review-pr-N, inner trees under W/.qwen/tmp/; the outer sandbox mounts P/.qwen/tmp read-write, so containerized code plants E = P/.qwen/tmp/.evil-git (config with filter.x.smudge) and rewrites an inner tree's gitfile to E. The inner gate's mountRootFor(inner tree) returns W/.qwen/tmp; E sits outside it, isSubpath false, the gate admits, and the subsequent worktree add / checkout --force resolves through E and runs its smudge on the host. Witness (probe at this commit against the real mountRootFor, flips solely with which mount root is consulted): mountRootFor(inner) = …/P/.qwen/tmp/review-pr-outer/.qwen/tmp; INNER GATE VERDICT: ADMIT (null); adminEntryInsideReviewTmp(E, …): false, judged against the ANCESTOR mount: true; SMUDGE CANARY CREATED: true. Fix: refuse an admin entry inside ANY .qwen/tmp ancestor of the tree (iterate every occurrence of the marker, or rule against the outermost) — a legitimate entry under <repo>/.git/worktrees/ is outside all of them, so no admit path changes; add a nested fixture test.
中文说明
[Critical] R8-4：包含判定只对照包含该树的最内层 .qwen/tmp 判定管理条目，但在 mountRootFor 自身文档所记载的嵌套审查几何里，树的 gitfile 可以经由每一层祖先 review 临时目录的沙箱挂载被改写——植入在祖先临时目录中的条目会被放行。失败场景：外层工作树 W = P/.qwen/tmp/review-pr-N，内层各树位于 W/.qwen/tmp/ 之下；外层沙箱以读写挂载 P/.qwen/tmp，容器化代码于是植入 E = P/.qwen/tmp/.evil-git（config 带 filter.x.smudge）并把内层树的 gitfile 改写为 E。内层门的 mountRootFor(内层树) 返回 W/.qwen/tmp；E 在其外，isSubpath 为假，门放行，随后 worktree add / checkout --force 经 E 解析并在宿主上执行其 smudge。证据（本提交上对真实 mountRootFor 的探针；判定仅随所对照的挂载根翻转）：mountRootFor(inner) = …/P/.qwen/tmp/review-pr-outer/.qwen/tmp；INNER GATE VERDICT: ADMIT (null)；adminEntryInsideReviewTmp(E, …): false，对照祖先挂载判定则为 true；SMUDGE CANARY CREATED: true。修复：拒绝位于该树任一 .qwen/tmp 祖先之内的管理条目（遍历标记的每次出现，或对照最外层判定）——<repo>/.git/worktrees/ 之下的合法条目在所有这些目录之外，放行路径不受影响；补一个嵌套夹具测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| const itWhereContainmentExists = it.skipIf(process.platform === 'win32'); | ||
|
|
||
| describe('untrustedGitfile', () => { |
There was a problem hiding this comment.
[Critical] R8-5: The new untrustedGitfile describe spawns real git (git init/commit/worktree add in pipelineTree()) without the isolateHostGitConfig() beforeEach that every sibling real-git suite in this same file installs — the suite is deterministically red on any host carrying commit.gpgsign=true without a usable key, the exact incident class the helper documents (polluted persistent-runner ~/.gitconfig, CI run 31516789251). Witness (A/B on the same suite at this commit): with a global git config containing only [commit] gpgsign = true — Tests 6 failed | 1 passed, every failure error: gpg failed to sign the data … fatal: failed to write commit object; the clean control passes 7/7, and the isolated sibling suite (worktreeResidue) passes 39/39 under identical pollution. This is the same shape as the R3-1/R6-3 lane-red Criticals this PR already paid for: deterministically red on a real host class (any developer who signs commits) for reasons the branch never touched. Fix: mirror the siblings — let gitIsolation: ReturnType<typeof isolateHostGitConfig>; with beforeEach(() => { gitIsolation = isolateHostGitConfig(); }) and gitIsolation.dispose() in the existing afterEach.
中文说明
[Critical] R8-5：新的 untrustedGitfile describe 通过 pipelineTree() 运行真实 git（git init/commit/worktree add），却没有同文件每个同类真实 git 套件都安装的 isolateHostGitConfig() beforeEach——在任何全局配置带 commit.gpgsign=true 而无可用密钥的宿主上，该套件必然红，正是该辅助函数所记录的事故类别（被污染的持久 runner ~/.gitconfig，CI 运行 31516789251）。证据（本提交上对同一套件的 A/B）：全局 git 配置仅含 [commit] gpgsign = true 时——Tests 6 failed | 1 passed，每条失败都是 error: gpg failed to sign the data … fatal: failed to write commit object；干净对照 7/7 通过；被隔离的兄弟套件（worktreeResidue）在相同污染下 39/39 通过。这与本 PR 已经付出过代价的 R3-1/R6-3 通道红灯 Critical 是同一形态：在一类真实宿主（任何签名提交的开发者）上因与本分支无关的原因必然红。修复：照兄弟套件补上——let gitIsolation: ReturnType<typeof isolateHostGitConfig>;，beforeEach(() => { gitIsolation = isolateHostGitConfig(); })，并在既有 afterEach 中 gitIsolation.dispose()。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const root = mountRoot(tree); | ||
| if (root === null) return null; |
There was a problem hiding this comment.
[Critical] R8-6: untrustedGitfile conflates mountRootFor's two nulls — "outside any review temp dir" (safe admit) and "temp root has a redirected/symlinked ancestor" (sandboxed-exec.ts returns null for exactly the danger case redirectedAncestor exists to name) — so replacing a nested review's .qwen/tmp root with a symlink silently disables EVERY gate this diff adds for every tree under it, and restoreProbeTreeTracked's directory-.git fall-through then turns the admission into host execution. Failure scenario: the outer containerized phase runs rm -rf W/.qwen/tmp && ln -s P/.qwen/tmp/.evil W/.qwen/tmp and stages .evil/<probe tree>/.git as a DIRECTORY repository of its own (config filter.evil.smudge, committed .gitattributes, own HEAD). Inner restoreProbeTreeTracked: leaf lstat passes (the symlink is a path prefix), the toplevel comparison passes (both sides resolve through the link), this gate gets null from mountRootFor and admits, lstatSync(T/.git).isFile() is FALSE so the whole identity branch is skipped, and checkout --force resolves through the planted repository — the -c inert pair covers hooks and fsmonitor but NOT filters. The revert gate admits the same null. Witness (probe ran the real compiled gate built at this commit): after the swap — mountRootFor(T): null, untrustedGitfile(T): null, and the exact restore checkout command created the canary in all three attacker-stageable tree states (A-missing-file/B-dirty-file/C-stale-stat: CANARY created=true); the control with -c filter.evil.smudge=true created nothing. Fix: fail closed on the second null (distinguish "outside any temp dir" from "redirected root" — a sentinel the gate treats as refusal — covering adminEntryInsideReviewTmp's identical null branch at line 233); independently, give restoreProbeTreeTracked the common-dir comparison resetScratchTree has for the directory-.git shape. Add a nested fixture test.
中文说明
[Critical] R8-13：untrustedGitfile 把 mountRootFor 的两种 null 混为一谈——「不在任何 review 临时目录之内」（可安全放行）与「临时根存在被重定向/符号链接的祖先」（sandboxed-exec.ts 恰为 redirectedAncestor 所要指认的危险情形返回 null）——于是把嵌套审查的 .qwen/tmp 根替换为符号链接，就能悄无声息地让本 diff 新增的每一道门对其下所有树失效，而 restoreProbeTreeTracked 对目录形 .git 的直通随后把放行变成宿主执行。失败场景：外层容器化阶段执行 rm -rf W/.qwen/tmp && ln -s P/.qwen/tmp/.evil W/.qwen/tmp，并把 .evil/<探针树>/.git 布置成一个独立的目录形仓库（config 带 filter.evil.smudge、提交过的 .gitattributes、自有 HEAD）。内层 restoreProbeTreeTracked：叶子 lstat 通过（符号链接是路径前缀），toplevel 比较通过（两侧都经链接解析），本门从 mountRootFor 得到 null 而放行，lstatSync(T/.git).isFile() 为 FALSE 使整个身份分支被跳过，checkout --force 经植入仓库解析——-c 惰性参数组覆盖 hooks 与 fsmonitor，但不覆盖 filter。revert 门对同一 null 同样放行。证据（探针运行按本提交构建的真实编译门）：替换之后——mountRootFor(T): null、untrustedGitfile(T): null，且 restore 的确切 checkout 命令在三种攻击者可布置的树状态下都创建了金丝雀（A-missing-file/B-dirty-file/C-stale-stat：CANARY created=true）；加上 -c filter.evil.smudge=true 的对照不产生任何文件。修复：对第二种 null 失败即关闭（区分「不在任何临时目录内」与「根被重定向」——一个被门视为拒绝的哨兵——并覆盖第 233 行 adminEntryInsideReviewTmp 相同的 null 分支）；独立地，给 restoreProbeTreeTracked 补上 resetScratchTree 对目录形 .git 所具备的 common-dir 比较。补一个嵌套夹具测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const untrusted = untrustedGitfile(probeTree, mountRootFor); | ||
| if (untrusted !== null) { | ||
| throw new Error(`refusing to revert: ${untrusted}`); | ||
| } |
There was a problem hiding this comment.
[Critical] R8-7: The new revert gate's location-only judgment admits a gitfile rewritten to name the repository's COMMON dir (the "main checkout" shape), and the revert path — unlike scratch reuse (resetScratchTree carries if (gitdir === commonOf(worktree)) return false; for exactly this measured-live shape) and probe restore — has no backpointer or gitdir≠commondir check downstream, so the guarded checkout writes the user's MAIN index, outside the throwaway tree. Failure scenario: during the probe phase (sandbox mounts .qwen/tmp read-write) reviewed code rewrites probeTree/.git to gitdir: <repo>/.git — attacker-known from the tree's original gitfile. restoreProbeTreeTracked declines that shape only incidentally (readFileSync(gitdir) ENOENT → inconclusive) and the mutation catch continues on purpose "so the revert probe below still runs". In revert, probeTargetEscapes passes (pure lstat walk), this gate ADMITS (rev-parse answers <repo>/.git, outside the temp dir), and git(probeTree, 'checkout', base, '--', …modified) resolves GIT_DIR=<repo>/.git with worktree=probeTree — staging the PR's modified files at BASE content into <repo>/.git/index, with stat info pointing into a tree the finally then discards: persistent, silent corruption of the user's main repository, against this comment's own promise that the phase is "confined to the throwaway tree". Witness (two-arm probe at this commit): ATTACK (gitfile → common dir): checkout exit 0 → main index entry moved to the base blob, main repo status MM a.txt, diff --cached: a.txt | 2 +-; FLIP (intact gitfile): checkout exit 0 → main index byte-identical, the write lands in the worktree's own index. The only variable between arms is the gitfile target. Fix: inside untrustedGitfile, when a mount exists, also query --git-common-dir from the same cwd and refuse when the resolved git dir equals it; independently give the revert path the backpointer round-trip restoreProbeTreeTracked applies. Add a fixture test (gitfile → common dir, expect refusal before the checkout).
中文说明
[Critical] R8-14：新的 revert 门只做位置判定，会放行被改写为指向仓库 COMMON dir 的 gitfile（「主检出」形态）；而 revert 路径——不同于 scratch 复用（resetScratchTree 恰为这一实测过的形态带着 if (gitdir === commonOf(worktree)) return false;）与探针恢复——下游没有任何 backpointer 或 gitdir≠commondir 检查，于是被守卫的 checkout 会写入一次性树之外用户的主索引。失败场景：探针阶段（沙箱以读写挂载 .qwen/tmp）被审代码把 probeTree/.git 改写为 gitdir: <repo>/.git——攻击者可从该树原有 gitfile 得知。restoreProbeTreeTracked 只是偶然拒绝该形态（readFileSync(gitdir) ENOENT → inconclusive），且变异 catch 有意继续「让下方的 revert 探针仍会运行」。进入 revert 后，probeTargetEscapes 通过（纯 lstat 行走），本门放行（rev-parse 回答 <repo>/.git，在临时目录之外），git(probeTree, 'checkout', base, '--', …modified) 以 GIT_DIR=<repo>/.git、worktree=probeTree 解析——把 PR 修改过的文件按 BASE 内容暂存进 <repo>/.git/index，stat 信息指向一棵 finally 随后就丢弃的树：对用户主仓库的持久而无声的损坏，与本注释自己「局限于一次性树」的承诺相悖。证据（本提交上的双臂探针）：攻击臂（gitfile → common dir）：checkout 退出码 0 → 主索引条目变为 base blob，主仓库状态 MM a.txt，diff --cached: a.txt | 2 +-；翻转臂（完好 gitfile）：checkout 退出码 0 → 主索引字节级不变，写入落在工作树自己的索引。两臂之间唯一的变量是 gitfile 指向。修复：在 untrustedGitfile 内，当存在挂载时，从同一 cwd 追加查询 --git-common-dir，解析出的 git dir 与之相等即拒绝；独立地给 revert 路径补上 restoreProbeTreeTracked 所应用的 backpointer 往返。补一个夹具测试（gitfile → common dir，期望在 checkout 之前被拒绝）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Last round's gate asked `untrustedGitfile` about `process.cwd()`, and that function demands `<tree>/.git` — right for the trees this pipeline builds, wrong for the directory a command was launched from. Any subdirectory of a checkout has no `.git` of its own; git walks up. So in the nested geometry the pipeline runs its own reviews in — cwd at `<review tree>/packages/cli` — the gate answered "has no .git to resolve" and fetch-pr aborted before the `worktree add` that would have succeeded. 77 tests in one suite, at the reviewer's HEAD. My checkout is not under `.qwen/tmp`, so `mountRootFor` returned null here and the gate never fired: the suite stayed green while the dogfood lane broke. That blind spot is now a test — a subdirectory of a mounted checkout, asserted to resolve. The launch directory gets its own question: ask git, from there, what repository `worktree add` will find, and judge only where that answer lives. No shape check, because the shape belongs to a tree and this is not one.
|
R8-1：我上一轮的修复打断了嵌套几何你说得完全对，而且我要补一条你没法看到的信息：这个回归在我的环境里是隐形的。
而我的检出不在 启动目录改用自己的问法：从那里问 git， 其余六条R8-2（ 我这一轮没有继续补，因为我认为再补下去帮不到这个 PR，理由是可测量的： | 轮次 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 而且 R8-2/R8-4/R8-6/R8-7 是这个形状的第五到第八次。逐条补下去，我预计下一轮还会是我自己修复的下游后果。 我建议的处理把这个 PR 拆开：
这个决定是维护者的，我不替它做主。在有决定之前我不会继续往本 PR 里加门——继续加只会让它更难评审，而不是更安全。 review + lease 全部套件 104 文件 5141 通过。tsc / lint / prettier 干净。 |
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.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- fetch-pr gate refusal test coverage gap — already recorded as D8-2/D8-3 (round-8 deferral list)
- scratch-tree double-poison fixture backpointer format — already recorded as D7-5 (round 7) and D8-6 (round 8)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/paths.ts:54 — [review] "nothing mounts it" is false in the nested geometry — inner leases land inside the outer review's RW mount
Convergence: round 9 posted 4 inline comment(s), 3 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/worktree.ts (findings in round 8; 2 more now); packages/cli/src/commands/review/lib/worktree.test.ts (findings in round 8; 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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 9 轮发布了 4 条行内评论，其中 3 条是首次提出；上一轮发布了 7 条（其中 7 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/worktree.ts（第 8 轮已出过发现，本轮又有 2 条）；packages/cli/src/commands/review/lib/worktree.test.ts（第 8 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * Returns a refusal, or null when there is nothing to police. Every host-side | ||
| * git command that WRITES resolves the repository through the `.git` of the | ||
| * tree it runs in, and for every tree this pipeline builds that `.git` sits |
There was a problem hiding this comment.
[Critical] R9-1: Class-level finding — the per-call-site gating of host-side git invocations that resolve through mount-resident rewritable .git pointers is structurally incomplete, and this round supersedes the six round-8 blockers (R7-2, R8-2, R8-3, R8-4, R8-6, R8-7) instead of re-posting them one by one. Every round for eight rounds has found either a new entrance the gate does not run before or a new divergence between the gate's judgment and git's own resolution, and the gate has been rewritten four times. Verified open at this HEAD: comment-status.ts runs four git -C <review worktree> probes (rev-parse / merge-base / git log) with no gate anywhere in the command, and git log executes a planted log.showSignature + gpg.program config on the host; the worktreeResidue status probes at scratch-tree.ts:517 and agent-prompt.ts:1376 remain ungated (round-8 probe created the host canary 3/3); base-tree.ts's gate runs AFTER discardWorktree resolves through the pointer, and the reuse fast path trusts entirely mount-resident state — a planted .qwen/tmp/review-pr-N-base (marker + attacker sources + a HEAD ref storing baseSha with no object behind it) passes the rev-parse check and corrupts the verifier A/B's merge-base measurement.
Witness (this round's verifier, driving the production makeGitProbe/gitOpt at this commit):
[A baseline (intact gitfile)] CANARY WRITTEN: false
[B planted common dir under the mount] gate="…admin entry is inside the review temp dir…"; CANARY WRITTEN: true
[D restored] CANARY WRITTEN: false
The class needs a structural close, not a ninth gate — your own proposal from the round-9 comment is the right shape: stop resolving host-side git through tree-resident writable pointers (pin --git-dir/--work-tree to host-trusted locations for every host-side spawn, or lift/restore the gitfile from the writable surface around the containerized phase), and stop trusting mount-resident state (verify a reused base tree against the repository's registered worktree admin entry instead of marker files).
中文说明
[Critical] R9-1：类级发现——对「经由挂载内可重写 .git 指针解析仓库的宿主侧 git 调用」做逐调用点门控，在结构上是不完备的；本轮以这一条取代第 8 轮的六条阻断项（R7-2、R8-2、R8-3、R8-4、R8-6、R8-7），不再逐条重发。八轮以来每一轮都发现一道门没有跑在它前面的新入口、或门的判定与 git 自身解析的一处新分歧，而门本身已被重写四次。在本 HEAD 上实测仍开放：comment-status.ts 的四条 git -C <review worktree> 探针（rev-parse / merge-base / git log）全程无任何门，git log 会执行植入的 log.showSignature + gpg.program 配置（宿主上执行）；scratch-tree.ts:517 与 agent-prompt.ts:1376 的 worktreeResidue status 探针仍无门（第 8 轮探针 3/3 写出宿主金丝雀）；base-tree.ts 的门在 discardWorktree 已经过该指针解析之后才运行，且 reuse 快速路径完全信任挂载内状态——植入的 .qwen/tmp/review-pr-N-base（marker + 攻击者源码 + 仅存 baseSha 而无对象的 HEAD ref）能通过 rev-parse 检查，从而腐蚀验证 A/B 的合并基线测量。
证据（本轮验证器在本提交上驱动生产代码 makeGitProbe/gitOpt）：基线臂金丝雀未写出；植入臂（门报「管理条目位于 review 临时目录内」）金丝雀写出；恢复臂金丝雀未写出。
这个类需要结构性收口，而不是第九道门——你在第 9 轮评论中提出的方案正是正确形状：让宿主侧 git 不再经由树内可写指针解析仓库（每次宿主侧 spawn 固定 --git-dir/--work-tree 到宿主可信位置，或在容器化阶段前后把 gitfile 从可写面上摘掉/复原），并且不再信任挂载内状态（reuse 的 base 树对照仓库已登记的 worktree 管理条目核验，而非信任 marker 文件）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const pipelineTree = () => { | ||
| const repo = tmp(); | ||
| const g = (cwd: string, ...args: string[]) => | ||
| execFileSync('git', args, { cwd, encoding: 'utf8' }).trim(); |
There was a problem hiding this comment.
[Critical] R8-5: Round-8 blocker, still standing at this HEAD (conceded in the round-9 comment): this new real-git describe spawns git init/commit/worktree add in pipelineTree() without the isolateHostGitConfig() beforeEach that every sibling real-git suite in this same file installs (worktreeResidue x3, discardWorktree). On any host carrying commit.gpgsign=true without a usable key — the exact incident class the helper documents — git commit -q -m init throws and the whole describe fails for reasons unrelated to the branch.
Witness (verifier probe at this commit):
poisoned HOME (gpgsign=true, no key): Tests 7 failed | 1 passed | 71 skipped
same HOME + isolation env vars: Tests 8 passed | 71 skipped
fix flip (add the beforeEach): Tests 8 passed | 71 skipped
let gitIsolation: ReturnType<typeof isolateHostGitConfig>;
beforeEach(() => {
gitIsolation = isolateHostGitConfig();
});
// and gitIsolation.dispose() in the existing afterEach中文说明
[Critical] R8-5：第 8 轮的阻断项在本 HEAD 上仍然成立（第 9 轮评论中已承认）：这个新的真跑 git 的 describe 在 pipelineTree() 里执行 git init/commit/worktree add，却没有同文件里每个同类套件都安装的 isolateHostGitConfig() beforeEach。在任何 commit.gpgsign=true 且没有可用密钥的宿主上——正是该辅助函数文档记录过的事故类别——git commit -q -m init 会抛错，整个 describe 因与分支无关的原因失败。证据（验证器在本提交上的探针）：污染 HOME 下 7 失败 / 1 通过；加上隔离环境变量后 8 通过；应用修复（补 beforeEach）后 8 通过。修复代码见上方英文部分。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const planted = join(tree, `\u00a0${real}`); | ||
| mkdirSync(dirname(planted), { recursive: true }); |
There was a problem hiding this comment.
[Critical] R9-4: This new test is deterministically red on the Windows lane. It builds the planted path by prefixing the ABSOLUTE gitdir target with U+00A0 and joining it under the tree — on Windows that target starts with a drive letter, so path.win32.join produces a <NBSP>C: segment (a colon inside a filename component), which NTFS rejects: mkdirSync throws EINVAL before any assertion runs. Its two equally platform-fragile siblings are gated with itWhereContainmentExists (win32 skip); this one was missed — the third time this lane class has surfaced in this PR (R3-1, R6-3). This PR is platform_sensitive (it touches the workflow definition and the scripts-test layer), so the lane runs on pull_request and merge_group: red on the PR page, and under merge_group the failure ejects the queue entry and stalls every PR batched with it.
Witness (verifier, Node's own win32 implementation):
path.win32.join(tree, '\u00a0' + real) → segment list contains ["\u00a0C:"]
Fix — plant via a relative NAME so the divergence stays pinned on every lane (write the gitfile gitdir: \u00a0planted-entry and create <tree>/<NBSP>planted-entry), or gate the test with itWhereContainmentExists like its siblings.
中文说明
[Critical] R9-4：这条新测试在 Windows 通道上必然失败。它把 U+00A0 前缀加在从 gitfile 读出的绝对 gitdir 目标上再 join 到树下——在 Windows 上该目标以盘符开头，path.win32.join 会产出 <NBSP>C: 这样的段（文件名成分里含冒号），NTFS 拒绝：mkdirSync 在任何断言运行前抛 EINVAL。它那两个同样平台脆弱的兄弟测试都用 itWhereContainmentExists（win32 跳过）守卫了，唯独这条漏掉——这是本 PR 第三次出现该通道类别（R3-1、R6-3）。本 PR 是 platform_sensitive（触及工作流定义与脚本测试层），所以该通道在 pull_request 与 merge_group 上都会运行：PR 页面直接变红，而在 merge_group 下失败会把队列条目踢出、卡住同批的所有 PR。证据（验证器，Node 自身的 win32 实现）：join 结果的段列表包含 ["\u00a0C:"]。修复——改用相对名字植入（gitfile 写 gitdir: \u00a0planted-entry，创建 <tree>/<NBSP>planted-entry），让该分歧测试在每个通道上成立；或像兄弟测试一样用 itWhereContainmentExists 守卫。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| real = realpathSync.native(resolve(gitDir)); | ||
| realRoot = realpathSync.native(root); |
There was a problem hiding this comment.
[Critical] R9-2: The new gate calls realpathSync.native, but fetch-pr.test.ts mocks node:fs.realpathSync as a plain vi.fn with no .native property. In any checkout whose path contains /.qwen/tmp/ — the nested/dogfood geometry this pipeline itself runs in, and the geometry the gate's own docstring documents — mountRootFor(process.cwd()) returns non-null, the gate activates, realpathSync.native(...) throws TypeError, and the fail-closed catch refuses everything with "refusing to create a review worktree: …resolves to an admin entry inside the review temp dir".
Witness (build-test measurement at this commit):
HEAD (nested geometry): 77 failures in src/commands/review/fetch-pr.test.ts
merge base a6d30ebc: same file green in the identical environment
test-delta: netNew = fetch-pr.test.ts (fails only on the PR side)
Plain-checkout CI never fires the gate, so this ships invisible while every nested run — including this pipeline's own scoped test runs — sees the suite red. The PR already updated this suite's mocks for the lease-path move; this new call site was missed:
realpathSync: Object.assign(vi.fn((p?: unknown): string => String(p)), {
native: vi.fn((p?: unknown): string => String(p)),
}),中文说明
[Critical] R9-2：新门调用 realpathSync.native，但 fetch-pr.test.ts 把 node:fs.realpathSync mock 成没有 .native 属性的普通 vi.fn。在任何路径包含 /.qwen/tmp/ 的检出里——正是本流水线自身运行的嵌套/自举几何，也是门的文档字符串明确记载的几何——mountRootFor(process.cwd()) 返回非 null，门启动，realpathSync.native(...) 抛 TypeError，fail-closed 的 catch 以「拒绝创建 review 工作树：……解析到 review 临时目录内的管理条目」拒绝一切。证据（本提交上的 build-test 实测）：HEAD（嵌套几何）fetch-pr.test.ts 77 条失败；同一环境下合并基线 a6d30eb 全绿；test-delta：netNew = fetch-pr.test.ts（仅在 PR 侧失败）。普通检出的 CI 从不触发该门，所以这个问题在合入前不可见，而每一次嵌套运行——包括本流水线自己的作用域测试运行——都会看到套件变红。本 PR 已经为 lease 路径迁移更新过该套件的 mock；这个新调用点被漏掉了。修复代码见上方英文部分。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…the fourth `realpathSync.native` is not always there. A suite that mocks `node:fs.realpathSync` as a bare `vi.fn` gives it no `.native`, and reaching through it threw a TypeError straight into the fail-closed catch — refusing every worktree creation in any checkout that happens to sit under `.qwen/tmp`, which is the geometry this pipeline reviews itself in. Falls back to the plain call, and a test asks that shape directly, because the suite that carries the mock never reaches this gate from an unmounted checkout. The real-git fixtures ran without `isolateHostGitConfig`, which every other real-git block in the same file installs. On a host with `commit.gpgsign=true` and no usable key the fixture commit fails and the whole block goes red for reasons unrelated to the gate — reproduced with a poisoned HOME. And one more case was left ungated for the Windows lane. Rather than gate it and wait for the fifth, the whole block is gated: every case in it builds a layout under `.qwen/tmp`, and on Windows neither the question nor the fixture exists. Worth recording why these three reached the branch at all: my checkout is not under `.qwen/tmp`, so `mountRootFor` answers null here and every one of these gates is inert on my machine. Green locally proves nothing about the geometry they are for.
|
三条都被我的环境掩盖，这一点值得单独说
我试着补一条同构验证腿（把仓库挂到 host git 隔离那条用被污染的 HOME（ Windows 那条我没有再逐条门控——逐条正是它在本 PR 里出现四次的原因。整个 block 一次门控：块内每条都在 R9-1你把它提为 class-level finding，我同意，而且这正是我上一轮停手的理由。 我第 8 轮给出的建议不变：lease 那一半是减法、七轮零 Critical，可以落地；gitfile 那一半需要的是结构性设计——让宿主侧 git 调用不再依赖树内可写的指针——而不是第九、第十道门。 在维护者给出范围决定之前，我只修我自己打进去的故障（像这一轮的三条），不再往里加新的门。 review + lease 全部套件 104 文件 5142 通过。tsc / lint / prettier 干净。 |
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.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- refusal-test coverage for untrustedRepositoryFrom, both fetch-pr gates and scratch's reuse gate — already reported (round-6 deferral D6-2; round-8 deferrals D8-2/D8-3; round-7/8 deferrals D7-5/D8-6)
- test coverage for untrustedGitfile's two fail-closed refusal branches — already reported (round-5 deferral D5-1; round-8 deferral D8-1)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 24757 passed.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 24757 passed。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| * Returns a refusal, or null when there is nothing to police. Every host-side | ||
| * git command that WRITES resolves the repository through the `.git` of the | ||
| * tree it runs in, and for every tree this pipeline builds that `.git` sits |
There was a problem hiding this comment.
[Critical] R9-1: Still standing at the round-10 HEAD — conceded in the PR thread, and the round-10 delta (the realpathSync.native fallback, the host-git isolation, the Windows-lane re-gating) does not address it. Class-level finding: the per-call-site gating of host-side git invocations that resolve through mount-resident rewritable .git pointers is structurally incomplete, and this class supersedes the six round-8 blockers (R7-2, R8-2, R8-3, R8-4, R8-6, R8-7) instead of re-posting them one by one. Verified open at this commit: comment-status.ts runs four git -C <review worktree> probes (rev-parse / merge-base / git log) with no gate anywhere in the command, and git log executes a planted log.showSignature + gpg.program config on the host; the worktreeResidue status probes at scratch-tree.ts:517 and agent-prompt.ts:1376 run BEFORE every gate this PR adds (re-read at this commit: the residue call at 517 precedes both the reuse gate near 562 and the rebuild gate near 609); base-tree.ts's gate runs AFTER discardWorktree resolves through the pointer, and the reuse fast path trusts entirely mount-resident state. New evidence this round: the verifier probe reproduced in a real nested geometry that the containment ruling asks only the INNERMOST .qwen/tmp, so a plant sited between the outer and inner mounts is admitted — the same entrance R8-4 named, folded into this class.
Witness (round-10 probe in a scratch tree; flips both ways):
outerMount = <outer>/.qwen/tmp
innerMount = <outer>/.qwen/tmp/review-pr-N/.qwen/tmp
adminEntryInsideReviewTmp(between-mounts plant, innerTree) = false
untrustedGitfile(innerTree) = null <- ADMITTED
same plant judged against the outer mount -> refused
The round-10 delta touches only worktree.ts and worktree.test.ts; every entrance above is byte-identical to the round-9 HEAD where the canary probe wrote on the host (planted arm: canary written; baseline and restored arms: not written).
The class needs a structural close, not a ninth gate — the shape your own round-9 comment proposed: stop resolving host-side git through tree-resident writable pointers (pin --git-dir/--work-tree to host-trusted locations for every host-side spawn, or lift/restore the gitfile from the writable surface around the containerized phase), and stop trusting mount-resident state (verify a reused base tree against the repository's registered worktree admin entry instead of marker files). Where containment is judged, walk EVERY .qwen/tmp ancestor, not just the innermost. The acceptance criterion for the close: an end-to-end canary test asserting nothing runs on the host when ANY tree points at a planted repository — including the between-mounts plant and the ungated status/log probes — and it must go red until the structural change lands.
中文说明
[Critical] R9-1：在第 10 轮 HEAD 上仍然存在——已在 PR 线程中承认，第 10 轮的增量（realpathSync.native 回退、宿主 git 隔离、Windows 通道重新门控）并未处理此条。类级发现：对「经由挂载内可重写 .git 指针解析仓库的宿主侧 git 调用」做逐调用点门控，在结构上不完备；本类取代第 8 轮的六条阻断项（R7-2、R8-2、R8-3、R8-4、R8-6、R8-7），不再逐条重发。在本提交上实测仍开放：comment-status.ts 的四条 git -C <review worktree> 探针（rev-parse / merge-base / git log）全程无门，git log 会执行植入的 log.showSignature + gpg.program 配置（宿主上执行）；scratch-tree.ts:517 与 agent-prompt.ts:1376 的 worktreeResidue status 探针跑在本 PR 所有门之前（本提交上复读确认：517 处的调用在约 562 的复用门与约 609 的重建门之前）；base-tree.ts 的门在 discardWorktree 已经过该指针解析之后才运行，且 reuse 快速路径完全信任挂载内状态。本轮新增证据：验证器探针在真实嵌套几何中复现——包含判定只问最内层 .qwen/tmp，因此植入在内外两层挂载之间的管理条目被放行——正是 R8-4 指出的入口，并入本类。
证据（第 10 轮探针，双向翻转）：外层挂载 = /.qwen/tmp；内层挂载 = /.qwen/tmp/review-pr-N/.qwen/tmp；adminEntryInsideReviewTmp(夹层植入条目, 内层树) = false；untrustedGitfile(内层树) = null（放行）；同一植入条目按外层挂载判定 → 拒绝。第 10 轮增量只改 worktree.ts 与 worktree.test.ts；上述每个入口与第 9 轮 HEAD 逐字节相同，而第 9 轮的金丝雀探针在植入臂写出了宿主文件（基线臂与恢复臂未写出）。
这个类需要结构性收口，而不是第九道门——你在第 9 轮评论中提出的方案正是正确形状：让宿主侧 git 不再经由树内可写指针解析仓库（每次宿主侧 spawn 固定 --git-dir/--work-tree 到宿主可信位置，或在容器化阶段前后把 gitfile 从可写面上摘掉/复原），并且不再信任挂载内状态（reuse 的 base 树对照仓库已登记的 worktree 管理条目核验，而非 marker 文件）。判定包含关系时应走遍每一层 .qwen/tmp 祖先，而不只是最内层。收口的验收标准：一条端到端金丝雀测试，断言任一棵树指向植入仓库时宿主上不执行任何东西——包括夹层植入与无门的 status/log 探针——在结构性改动落地之前它必须是红的。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…-out-of-mount Resolve packages/cli/src/commands/review/lib/paths.ts: keep main's generated-workflows block (GENERATED_WORKFLOWS_SUBDIR through reviewWorkflowScriptPath) and apply only this branch's LEASE_PREFIX doc change from REVIEW_TMP_DIR to REVIEW_LEASE_DIR.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- fetch-pr gate refusal-test coverage gap — already recorded as D6-2 (round-6 deferral) and D8-2/D8-3 (round-8 deferrals)
- NBSP comment/fixture mismatch at worktree.test.ts:1957 — already recorded as D6-3 (round-6 deferral)
- case-variant realpathSync.native witness gap — already recorded as D7-1 (round-7 deferral)
- scratch-tree both-trees backpointer fixture vacuous reuse witness — already recorded as D7-5 (round-7) and D8-6 (round-8) deferrals
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally.
Not explored to full depth (tool budget reached): "agent 1b": none — all checks above ran to completion..
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory.
Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round:
packages/cli/src/services/review-worktree-lease.ts:155 — [probe] permission-poisoned legacy lease directory still wedges every later review of that PR (base wedged worse — hardening gap, not a regression)scripts/tests/review-worktree-cleanup-workflow.test.js:499 — [probe] the two new lease-sweep lines are pinned only by toContain; the executed harness never plants a lease
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (1 Critical(s)), the rate of first-time findings is not falling (this round 0, previous 0), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1b"：none — all checks above ran to completion.。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory。
收敛姿态下延后（第 11 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 1 条 Critical），首次发现的速率没有下降（本轮 0，上一轮 0），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * | ||
| * Returns a refusal, or null when there is nothing to police. Every host-side | ||
| * git command that WRITES resolves the repository through the `.git` of the | ||
| * tree it runs in, and for every tree this pipeline builds that `.git` sits |
There was a problem hiding this comment.
[Critical] R9-1: The per-call-site gating of host-side git invocations that resolve through mount-resident rewritable .git pointers is still structurally incomplete — standing at the round-11 HEAD, conceded in the PR thread. The PR's own code is byte-identical to round 10 (only a merge of main landed), and every entrance below was re-verified ungated at this commit.
Concretely, at this commit: the worktreeResidue status probes (scratch-tree.ts:517, agent-prompt.ts:1376) run BEFORE every gate this PR adds — a status index refresh executes a planted clean/process filter on the host. comment-status.ts's four git -C <review worktree> probes carry no gate — git log executes a planted log.showSignature + gpg.program config on the host. fetch-pr's step-2 git fetch runs before the step-4 launch-dir gate and executes config-planted commands; a fresh probe at this HEAD confirmed a planted core.sshCommand ran on the host during the fetch. And the containment ruling asks only the INNERMOST .qwen/tmp, admitting a plant sited between nested mounts.
This class supersedes R7-2, R8-2, R8-3, R8-4, R8-6, R8-7 and carries R6-4's check-then-act window — the structural close R9-1 prescribes is exactly R6-4's proven fix arm.
Witness:
round-10 probe (flips both ways): adminEntryInsideReviewTmp(between-mounts plant) = false;
untrustedGitfile(innerTree) = null <- ADMITTED; same plant judged against the outer mount -> refused.
round-11 re-check at ae424f11: every entrance still ungated; fresh probe: `git fetch` under a
planted core.sshCommand executed the planted command on the host.
The class needs a structural close, not a ninth gate (the shape proposed in the round-9 comment): pin --git-dir/--work-tree to host-trusted locations for every host-side spawn, or lift/restore the gitfile from the writable surface around the containerized phase, and stop trusting mount-resident state. Where containment is judged, walk EVERY .qwen/tmp ancestor, not just the innermost.
Acceptance criterion: an end-to-end canary test asserting nothing runs on the host when ANY tree points at a planted repository — including the between-mounts plant and the ungated status/log probes; it must stay red until the structural change lands.
中文说明
[Critical] R9-1：「对经由挂载内可重写 .git 指针解析仓库的宿主侧 git 调用」做逐调用点门控，在结构上仍然不完备——在第 11 轮 HEAD 上依然存在，且已在 PR 线程中承认。本 PR 自身代码与第 10 轮逐字节相同（仅合入了 main），下述每个入口都在本提交上复验为仍未设防。
具体而言：worktreeResidue 的 status 探针（scratch-tree.ts:517、agent-prompt.ts:1376）跑在本 PR 所有门之前——status 的索引刷新会在宿主上执行植入的 clean/process 过滤器。comment-status.ts 的四条 git -C <review worktree> 探针全程无门——git log 会在宿主上执行植入的 log.showSignature + gpg.program 配置。fetch-pr 的 step-2 git fetch 跑在 step-4 启动目录门之前，会执行配置植入的命令；本轮在本 HEAD 上的新探针确认：植入的 core.sshCommand 在 fetch 期间于宿主上执行。且包含判定只问最内层 .qwen/tmp，因此植入在嵌套挂载夹层之间的管理条目会被放行。
本类取代 R7-2、R8-2、R8-3、R8-4、R8-6、R8-7，并承载 R6-4 的检查后行动窗口——R9-1 所要求的结构性收口正是 R6-4 已被证明的修复臂。
这个类需要结构性收口，而不是第九道门（即你在第 9 轮评论中提出的形状）：每次宿主侧 spawn 固定 --git-dir/--work-tree 到宿主可信位置，或在容器化阶段前后把 gitfile 从可写面上摘掉/复原，并且不再信任挂载内状态。判定包含关系时应走遍每一层 .qwen/tmp 祖先，而不只是最内层。
验收标准：一条端到端金丝雀测试，断言任一棵树指向植入仓库时宿主上不执行任何东西——包括夹层植入与无门的 status/log 探针；在结构性改动落地之前它必须保持为红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- fetch-pr gate refusal-test coverage gap (both fetch-pr gates and untrustedRepositoryFrom's refusal branch have zero witnesses) — already recorded as D6-2 (round 6) and D8-2/D8-3 (round 8) deferrals
- NBSP comment/fixture mismatch at worktree.test.ts:1957 — already recorded as D6-3 (round 6)
- scratch-tree both-trees backpointer fixture vacuous reuse witness — already recorded as D7-5 (round 7) and D8-6 (round 8)
- case-variant realpathSync.native witness gap at worktree.test.ts:2065 — already recorded as D7-1 (round 7), re-confirmed duplicate in round 11
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 26305 passed.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/test-efficacy.integration.test.ts:410 — [probe] D12-1: restore-gate integration test is vacuous — planted backpointer uses the gitdir: -prefixed shape git never writes; gate-removal mutation ships green un…packages/cli/src/services/review-worktree-lease.test.ts:408 — [probe] D12-2: legacy-lease removal witness pins only the directory wedge; an isDirectory()-guard mutant survives 23/23 — the plain-file shape needs its own case
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (1 Critical(s)), the rate of first-time findings is not falling (this round 0, previous 0), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory; 5077 passing — this review observed 26305 passed。
收敛姿态下延后（第 12 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 1 条 Critical），首次发现的速率没有下降（本轮 0，上一轮 0），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| /** | ||
| * Why a tree's own gitfile cannot be trusted to resolve a host-side git write. | ||
| * | ||
| * Returns a refusal, or null when there is nothing to police. Every host-side |
There was a problem hiding this comment.
[Critical] R9-1: [certifies-falsely] Round-12 re-post of the standing class-level finding (rounds 9-11), conceded in the PR thread, re-probed at this commit: the per-call-site gating of host-side git invocations that resolve through mount-resident rewritable .git pointers is structurally incomplete. The PR's own code is byte-identical to round 11 (only a merge of main landed since). Four entrances re-verified at this commit: (1) worktreeResidue at scratch-tree.ts:517 and agent-prompt.ts:1383 runs its pinned git status probe BEFORE the reuse/rebuild gates this PR adds — with a coherent plant (bare-path backpointer, filter.*.process in the planted common dir) the identity checks pass and the status's index refresh executes the planted filter on the host; (2) fetch-pr's fresh path runs cleanStale (:929) and :935) before the launch-dir gate (~:1013), and git fetch (git fetch through the planted pointer executes planted transport config; (3) comment-status.ts runs four ungated git -C <review worktree> probes (:276/:301/:319/:414) through the rewritable pointer (reads; no execution mechanism established for those commands); (4) the containment predicate judges only against the INNERMOST .qwen/tmp root — in the nested geometry the pipeline documents, a between-roots plant inside the outer RW mount is admitted by every gate and the guarded worktree add executes the planted filter.
Failure scenario: the outer review's containerized build/test phase rewrites <repo>/.qwen/tmp/review-pr-N/.git to a forged admin entry planted under the same read-write mount; a subsequent pipeline invocation — the residue probe, the fresh fetch path, or any gate judging against the innermost root only — resolves through it: host-side code execution as the review user, with CI runner tokens in reach, inside the very class this diff's docstring claims to close.
Witness:
residue route INTACT PR: {"canaryWritten":true, "sharedTreeUnmeasured":"git status exited 128"}
WITH FIX: {"canaryWritten":false, "sharedTreeUnmeasured":"the command refused before it measured the shared worktree"}
fetch arm planted core.sshCommand canary created by `git fetch origin` through the planted pointer (exit 128)
between-roots OBS gate verdict = null, worktree add status = 0, canary exists = true, RESULT: BYPASS
outermost-root flip: gate refused, canary absent
Suggested fix — close the class structurally rather than entrance by entrance: run the location gate before worktreeResidue measures (scratch-tree.ts:517, agent-prompt.ts:1383), treating a refusal as unmeasured-with-reason; move untrustedRepositoryFrom(process.cwd(), mountRootFor) to the top of fetch-pr's fresh path, before cleanStale; and widen adminEntryInsideReviewTmp's distrust judgement to ANY ancestor review temp root. Alternatively this is the round-11 land-with-residual-risk decision for a maintainer — but that decision should name these four entrances.
The fix must keep the MOUNT root innermost — sandboxed-exec.ts:446-452: lastIndexOf, because the FIRST occurrence would widen the mount to the outer temp dir, pulling <repo>/.git and every sibling checkout in with it; only the distrust judgement may widen. scratch-tree.ts:346-352 documents the accepted TOCTOU residual window the fix must not be mistaken for closing. A coherent-plant property test using git's real bare-path backpointer format — the committed fixtures write a gitdir: -prefixed backpointer git never writes, which lets the round-trip decline before the policed spawn and masks every route above — must keep the canary absent with all gates intact, and go red when the entrance under test is ungated (remove that gate and run the test; confirm it reds).
中文说明
[Critical] R9-1：第 12 轮重新发布这一自第 9 轮起持续存在的类级发现（第 9-11 轮已发布），作者已在 PR 讨论中承认；本轮在该提交上重新以探针验证：对「经由挂载内可改写的 .git 指针解析仓库」的宿主侧 git 调用，其逐调用点门控在结构上不完备。本 PR 自身代码与第 11 轮逐字节一致（此后仅合入过一次 main）。本提交上复核的四处入口：(1) scratch-tree.ts:517 与 agent-prompt.ts:1383 处的 worktreeResidue 在本 PR 新增的复用/重建门之前运行其钉死的 git status 探针——对于自洽的植入（裸路径 backpointer、植入 common dir 中带 filter.*.process），身份检查全部通过，status 的索引刷新会在宿主上执行植入的过滤器；(2) fetch-pr 全新路径在启动目录门（约 :1013）之前运行 cleanStale（约 :929）与 git fetch（约 :935），经由植入指针的 git fetch 会执行植入的传输配置；(3) comment-status.ts 有四处未设门的 git -C <review worktree> 探针（:276/:301/:319/:414）经由可改写指针读取（未证实这些命令有执行机制）；(4) 包含判定只对照最内层 .qwen/tmp 根——在流水线文档明确支持的嵌套几何里，位于外层读写挂载之内、内层根之外的植入会被所有门放行，随后受门保护的 worktree add 执行植入的过滤器。
失败场景：外层审查的容器化构建/测试阶段把 <repo>/.qwen/tmp/review-pr-N/.git 改写为同一读写挂载下植入的伪造管理条目；随后的流水线调用——残留探针、全新 fetch 路径、或任何只对照最内层根判定的门——经由它解析：以审查用户身份在宿主上执行代码，在 CI 上可触及 runner 令牌，恰好发生在本 diff 文档字符串声称要关闭的类别之内。
证据（本提交上的探针）：残留路径——原样 PR {"canaryWritten":true, "sharedTreeUnmeasured":"git status exited 128"}；应用修复后 {"canaryWritten":false, "sharedTreeUnmeasured":"the command refused before it measured the shared worktree"}；fetch 臂——经由植入指针的 git fetch origin 写出了植入的 core.sshCommand 金丝雀（exit 128）；根间臂——门判定为 null、worktree add 退出 0、金丝雀存在，结果为 BYPASS；改为对照最外层根后：门拒绝、金丝雀不存在。
建议修复——以结构化方式关闭整个类别，而不是逐入口打补丁：让 worktreeResidue 在测量前先过位置门（scratch-tree.ts:517、agent-prompt.ts:1383），拒绝时记为「带原因的未测量」；把 untrustedRepositoryFrom(process.cwd(), mountRootFor) 移到 fetch-pr 全新路径最前端、cleanStale 之前；把 adminEntryInsideReviewTmp 的不信任判定放宽到任意祖先 review 临时根。或者，按第 11 轮的 land-with-residual-risk 建议，由 maintainer 做风险接受决定——但该决定应点名上述四处入口。
修复必须保持挂载根为最内层——见 sandboxed-exec.ts:446-452：使用 lastIndexOf，因为取第一次出现会把挂载放宽到外层临时目录，把 <repo>/.git 和所有兄弟检出都拉进来；只有不信任判定可以放宽。scratch-tree.ts:346-352 记录了已被接受的 TOCTOU 残留窗口，修复不应被误认为关闭了它。需要一条使用 git 真实裸路径 backpointer 格式的自洽植入属性测试——已提交的夹具写的是 git 从不写出的 gitdir: 前缀 backpointer，使往返检查在被门控的 spawn 之前就拒绝，从而掩盖了上述所有路径——该测试必须在所有门完好时保持金丝雀不存在，并在被测入口被解除门控时变红（移除该门并运行该测试，确认变红）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
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.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- fetch-pr gate refusal-test coverage gap (both fetch-pr gates and untrustedRepositoryFrom's refusal branch have zero witnesses) — already recorded as D6-2 (round 6) and D8-2/D8-3 (round 8) deferrals
- scratch both-trees backpointer fixture / reuse-gate pairing vacuity (gitdir: -prefixed backpointer shape git never writes) — already recorded as D7-5 (round 7) and D8-6 (round 8)
- NBSP comment/fixture mismatch at worktree.test.ts:1957 — already recorded as D6-3 (round 6)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): src/services/review-worktree-lease.test.ts — no such file or directory.
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
packages/cli/src/services/review-worktree-lease.test.ts:408 — [probe] D13-1: legacy-lease removal witness pins the file and directory shapes but not a symlink — Node unlinks without following today, yet a future link-following rewrite ships…
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (1 Critical(s)), the rate of first-time findings is not falling (this round 0, previous 0), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge_group only) and the macOS arm did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge_group only) and the Windows arm did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
Test Plan（非阻断）：src/services/review-worktree-lease.test.ts — no such file or directory。
收敛姿态下延后（第 13 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 1 条 Critical），首次发现的速率没有下降（本轮 0，上一轮 0），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| /** | ||
| * Why a tree's own gitfile cannot be trusted to resolve a host-side git write. | ||
| * | ||
| * Returns a refusal, or null when there is nothing to police. Every host-side |
There was a problem hiding this comment.
[Critical] R9-1: [certifies-falsely] Round-13 re-post of the standing class-level finding (rounds 9-12), conceded in the PR thread, re-probed at this commit (code byte-identical to round 12): the per-call-site gating of host-side git invocations that resolve through mount-resident rewritable .git pointers is structurally incomplete. Four entrances re-verified this round: (1) worktreeResidue at scratch-tree.ts:517 and agent-prompt.ts:1383 runs BEFORE the reuse/rebuild gates this PR adds — with a coherent plant (bare-path backpointer, filter.*.process in the planted common dir) the status index refresh executes the planted filter on the host; (2) fetch-pr's fresh path runs cleanStale (:929) and :935) before the launch-dir gate (~:1013), and git fetch (git fetch through the planted pointer executes planted transport config; (3) comment-status.ts runs four ungated git -C <review worktree> probes (:276/:301/:319/:414) through the rewritable pointer (reads; no execution mechanism established for those commands); (4) the containment predicate judges only against the INNERMOST .qwen/tmp root — in the nested geometry the pipeline documents, a between-roots plant inside the outer RW mount is admitted by every gate and the guarded worktree add executes the planted filter.
Failure scenario: the outer review's containerized build/test phase rewrites <repo>/.qwen/tmp/review-pr-N/.git to a forged admin entry planted under the same read-write mount; a subsequent pipeline invocation — the residue probe, the fresh fetch path, or any gate judging against the innermost root only — resolves through it: host-side code execution as the review user, with CI runner tokens in reach, inside the very class this diff's docstring claims to close.
Witness:
P1 residue route — INTACT PR (production runScratchTree, all gates intact): canary existsSync === true
— WITH FIX: AssertionError: expected false to be true (canary absent — refusal before measuring)
P2 between-roots — untrustedGitfile(t2, mountRootFor) === null (admitted); worktree add exit 0; canary written
— outermost-root flip: refusal "…admin entry is inside the review temp dir…"
P3 fetch arm — git fetch through the planted pointer: core.sshCommand canary written, exit 128
Suggested fix — close the class structurally rather than entrance by entrance: run the location gate before worktreeResidue measures (scratch-tree.ts:517, agent-prompt.ts:1383), treating a refusal as unmeasured-with-reason; move untrustedRepositoryFrom(process.cwd(), mountRootFor) to the top of fetch-pr's fresh path, before cleanStale; and widen adminEntryInsideReviewTmp's distrust judgement to ANY ancestor review temp root. Alternatively this is the round-11/12 land-with-residual-risk decision for a maintainer — but that decision should name these four entrances.
The fix must keep the MOUNT root innermost — sandboxed-exec.ts:446-452: lastIndexOf, because the FIRST occurrence would widen the mount to the outer temp dir, pulling <repo>/.git and every sibling checkout in with it; only the distrust judgement may widen. scratch-tree.ts:346-352 documents the accepted TOCTOU residual window the fix must not be mistaken for closing. A coherent-plant property test using git's real bare-path backpointer format — the committed fixtures write a gitdir: -prefixed backpointer git never writes, which lets the round-trip decline before the policed spawn and masks every route above — must keep the canary absent with all gates intact, and go red when the entrance under test is ungated (remove that gate and run the test; confirm it reds).
中文说明
[Critical] R9-1：第 13 轮重新发布这一自第 9 轮起持续存在的类级发现（第 9-12 轮已发布），作者已在 PR 讨论中承认；本轮在该提交上重新以探针验证（代码与第 12 轮逐字节一致）：对「经由挂载内可改写的 .git 指针解析仓库」的宿主侧 git 调用，其逐调用点门控在结构上不完备。本轮复核的四处入口：(1) scratch-tree.ts:517 与 agent-prompt.ts:1383 处的 worktreeResidue 在本 PR 新增的复用/重建门之前运行——对于自洽的植入（裸路径 backpointer、植入 common dir 中带 filter.*.process），status 的索引刷新会在宿主上执行植入的过滤器；(2) fetch-pr 全新路径在启动目录门（约 :1013）之前运行 cleanStale（约 :929）与 git fetch（约 :935），经由植入指针的 git fetch 会执行植入的传输配置；(3) comment-status.ts 有四处未设门的 git -C <review worktree> 探针（:276/:301/:319/:414）经由可改写指针读取（未证实这些命令有执行机制）；(4) 包含判定只对照最内层 .qwen/tmp 根——在流水线文档明确支持的嵌套几何里，位于外层读写挂载之内、内层根之外的植入会被所有门放行，随后受门保护的 worktree add 执行植入的过滤器。
失败场景：外层审查的容器化构建/测试阶段把 <repo>/.qwen/tmp/review-pr-N/.git 改写为同一读写挂载下植入的伪造管理条目；随后的流水线调用——残留探针、全新 fetch 路径、或任何只对照最内层根判定的门——经由它解析：以审查用户身份在宿主上执行代码，在 CI 上可触及 runner 令牌，恰好发生在本 diff 文档字符串声称要关闭的类别之内。
证据（本提交上的探针）：P1 残留路径——原样代码（生产 runScratchTree，所有门完好）金丝雀写出；应用修复后 expected false to be true（金丝雀不存在——测量前即拒绝）。P2 根间植入——untrustedGitfile 判定为 null（放行），worktree add 退出 0，金丝雀写出；改为对照最外层根后拒绝。P3 fetch 臂——经由植入指针的 git fetch 写出 core.sshCommand 金丝雀（exit 128）。
建议修复——以结构化方式关闭整个类别而不是逐入口打补丁：让 worktreeResidue 在测量前先过位置门（scratch-tree.ts:517、agent-prompt.ts:1383），拒绝时记为「带原因的未测量」；把 untrustedRepositoryFrom(process.cwd(), mountRootFor) 移到 fetch-pr 全新路径最前端、cleanStale 之前；把 adminEntryInsideReviewTmp 的不信任判定放宽到任意祖先 review 临时根。或者，按第 11/12 轮的 land-with-residual-risk 建议，由 maintainer 做风险接受决定——但该决定应点名上述四处入口。
修复必须保持挂载根为最内层——见 sandboxed-exec.ts:446-452：使用 lastIndexOf，因为取第一次出现会把挂载放宽到外层临时目录，把 <repo>/.git 和所有兄弟检出都拉进来；只有不信任判定可以放宽。scratch-tree.ts:346-352 记录了已被接受的 TOCTOU 残留窗口，修复不应被误认为关闭了它。需要一条使用 git 真实裸路径 backpointer 格式的自洽植入属性测试——已提交的夹具写的是 git 从不写出的 gitdir: 前缀 backpointer，使往返检查在被门控的 spawn 之前就拒绝，从而掩盖了上述所有路径——该测试必须在所有门完好时保持金丝雀不存在，并在被测入口被解除门控时变红（移除该门并运行该测试，确认变红）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
3 participants
What this PR does
Moves the pipeline's worktree lease files out of the directory the review sandbox bind-mounts read-write, and teaches the host-side probe-tree restore to refuse a worktree admin entry that resolves inside that directory.
Why it's needed
Two findings from #9723's review — R19-1 and R1-3 — are the same root cause: state the pipeline later trusts lives in the directory it hands the container read-write. They need one fix, not two.
Neither is opened by the sandbox. On
maintoday the reviewed repository's commands run as the host user with no filesystem restriction at all, so both are reachable there and reachable more widely; containment narrowed the reach rather than creating it. What it did not do is close it, and arequiredthat claims containment should not leave host-trusted state inside the writable surface.Leases.
cleanupReviewWorktreeLeasesmatches a lease by(sessionId, promptId, repositoryRoot)alone — no provenance check — and then force-removes whatever worktree and deletes whatever branch it names. The files lived in the review temp dir. Reviewed code that edits one makes another session's cleanup destroy the wrong tree; one it plants with a foreign session id is swept by nobody and wedges that PR on that machine until an operator deletes the file by hand.Gitfiles. A pipeline tree's
.gitcannot move — git requires it at<tree>/.git— so the check moves instead. The identity gates that already exist cannot see this attack and never could: the writer chooses both the gitfile's target and that target's backpointer, so the round-trip agrees with itself;--show-toplevelstill prints the tree the gitfile sits in; no symlink is involved anywhere; and asking git for the common dir resolves through the rewritten gitfile, so that answer comes from the same hand. Location is the question that does have an honest answer: a real linked worktree's admin entry lives under<repo>/.git/worktrees/, outside the mount, and a planted one has to be inside it.Reviewer Test Plan
How to verify
Unit tests, from the repository root:
To see the attack the gate exists for, build it by hand — a rewritten gitfile pointing at a planted admin entry whose own
commondirnames a planted repository, both under<repo>/.qwen/tmp:Evidence (Before & After)
Measured on the fixture above, before the fix:
--show-toplevelequals the probe treegit checkout --forceon the hostPWNEDwritten — the plantedfilter.evil.smudgeexecutedAfter: the same shape is refused by
adminEntryInsideReviewTmp, while the real admin entry it replaced still passes. Both assertions run against the fixture built by that script.Mutation evidence — three mutants, three reds: a gate that always admits; one that admits when the entry cannot be resolved (fail-open on the direction that ends in host execution); one that refuses when there is no mount at all, which would refuse every ordinary repository.
Lease half: the suite pinned the old location, so the move reddens it; the tests now name
.qwen/review-leases. Full review + lease suites: 104 files, 5077 passing.Tested on
Environment (optional)
Unit tests under vitest; the reproduction uses only
git. No container runtime required for either.Risk & Scope
restoreProbeTreeTracked, the consumer with a demonstrated end-to-end reproduction. Other host-side git paths (scratch-tree setup, base-tree build) carry their own identity gates and are not changed here; whether they need the same location check is worth a separate look rather than a speculative edit. Windows and Linux not exercised locally.Linked Issues
Addresses R19-1 and R1-3 from #9723's review, both disclosed in that PR's description as not closed there. Related: #9556.
中文说明
本 PR 做了什么
把流水线的工作树 lease 文件移出 review 沙箱以读写方式绑定挂载的那个目录，并让宿主侧的探针树恢复拒绝「解析后位于该目录之内」的工作树管理条目。
为什么需要
#9723 评审里的两条发现 —— R19-1 与 R1-3 —— 是同一个根因：流水线事后信任的状态，存放在它交给容器读写的那个目录里。它们需要一次修复，而不是两次。
两条都不是沙箱打开的。在今天的
main上，被审仓库的命令以宿主用户身份、在毫无文件系统限制的情况下运行，因此那里同样够得到、而且够得更宽；容器化收窄了可达范围，而非制造了它。但它确实没有关掉这两条，而一个声称容器化的required不应把宿主可信状态留在可写面之内。Lease。
cleanupReviewWorktreeLeases仅凭(sessionId, promptId, repositoryRoot)匹配 —— 没有任何来源校验 —— 随后对 lease 中所写的工作树与分支执行强制删除。这些文件此前就在 review 临时目录里。被审代码修改其中一个，就能让另一个会话的清理销毁错误的树；植入一个带外来 session id 的 lease，则无人会清扫，该 PR 在这台机器上被永久卡住，直到有人手工删除该文件。Gitfile。 流水线树的
.git无法移动 —— git 要求它位于<tree>/.git—— 所以移动的是检查。已有的身份门看不见这个攻击，而且本来就不可能看见：写入者同时选择了 gitfile 的指向与该指向处的 backpointer，因此往返自洽；--show-toplevel仍然打印 gitfile 所在的那棵树；全程不涉及符号链接；而向 git 询问 common dir，其解析经过被改写的 gitfile，因此那个答案出自同一只手。位置才是有诚实答案的问题：真实 linked worktree 的管理条目位于<repo>/.git/worktrees/之下、在挂载之外，而植入的那个必须在挂载之内。评审者验证方案
如何验证
单元测试，在仓库根目录执行：
要看这道门所针对的攻击，可手工构造 —— 被改写的 gitfile 指向一个植入的管理条目，而该条目自己的
commondir又指向一个植入的仓库，两者都在<repo>/.qwen/tmp之内（脚本见上方英文部分）。证据（前后对比）
在上述夹具上实测，修复前：
--show-toplevel等于探针树git checkout --forcePWNED—— 植入的filter.evil.smudge被执行修复后：同一形态被
adminEntryInsideReviewTmp拒绝，而它所替换掉的真实管理条目仍然通过。两条断言都跑在该脚本构建的夹具上。变异证据 —— 三个变异，三次红：恒放行的门；解析失败时放行的门（在会通向宿主执行的那个方向上 fail-open）；无挂载时也拒绝的门 —— 最后这个会拒掉每一个普通仓库。
Lease 那一半：套件此前钉住了旧位置，所以移位会打红它们；测试已改为
.qwen/review-leases。review + lease 全部套件：104 个文件、5077 通过。测试平台
运行环境（可选）
单元测试在 vitest 下运行；复现脚本只用到
git。两者都不需要容器运行时。风险与范围
restoreProbeTreeTracked上，即已有端到端复现的那个消费者。其他宿主侧 git 路径（scratch-tree 建立、base-tree 构建）各自带有自己的身份门，本 PR 未作改动；它们是否需要同样的位置检查，值得单独考察，而不是凭猜测顺手改。Windows 与 Linux 未在本地实测。关联 Issue
处理 #9723 评审中的 R19-1 与 R1-3，两者都已在该 PR 的描述中披露为「本 PR 关不掉的」。相关：#9556。