fix(review): repair permissions before giving up on worktree cleanup - #9748
Conversation
The review job's end-of-job sweep gave up on the first EACCES and left foreign-owned leftovers in the shared runner workspace; the next review's checkout then died on them (run 32577821716, PR #9718: a scratch-verify tree whose contents this job's user could not unlink, on a pool member without passwordless sudo). Give the removal a repair ladder — chmod what this user owns, then passwordless sudo chown/chmod where the pool member has it, each followed by a retry — and refuse the ladder on paths that resolve through symlinks, since its sudo leg escalates to root. Members without sudo still degrade to a named warning: nothing unprivileged can remove a foreign-owned tree, but the sweep must never fail the job. Pin the ladder in the cleanup contract test so a rewrite cannot silently drop it back to warn-and-leave.
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
…w size baseline The permission-repair ladder added to the review cleanup step (repair before giving up on a worktree removal, refuse the sudo leg through symlinks) plus its incident comments grew the file past its recorded size plus allowance. The growth is the fix itself — the repair logic and the rationale a future reader needs — not drift, so record the shipped size rather than trimming the rationale.
|
Workflow size baseline bumped in 04ace8a: the permission-repair ladder and its incident comments in the cleanup step grew qwen-code-pr-review.yml from 151937 to 156817 bytes (allowance is 4096). The growth is the fix itself — repair logic plus the rationale a future reader needs — so recorded rather than trimmed. Gate script and the workflow-size test suite pass locally. |
|
Re-run on explicit
Moving on to code review. 🔍 中文说明由 @wenshao 显式
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe ladder's structure holds up under scrutiny: initial One blocker — the same one the round-7 review raised, verified independently against this commit rather than taken on trust:
Non-blocking:
Testing evidence (unattended run — the PR's own CI via API; no PR code executed by triage)
All checks on this head are complete; the green ubuntu unit lane includes this PR's contract + fixture suite, which executes Real-scenario (tmux): N/A — CI-workflow-only change, nothing user-visible. The synthetic-workspace results in the PR body are the author's claim, not independently re-run here. 中文说明代码评审梯子的结构经得起推敲：初次 一个阻断项——与第 7 轮评审所提相同，但本次是针对本提交独立核实的，不是照搬：
非阻断：
测试证据（无人值守运行——通过 API 读取 PR 自身 CI；triage 不执行 PR 代码）（CI 表格见上方英文部分，结论不变：ubuntu 单元通道绿色且包含本 PR 的契约 + 夹具套件；macOS/Windows 通道按惯例跳过；全部检查已完成。） 未验证：夹具无法构造外部属主的树，因此 sudo 级只有结构性 pin、从未被执行——真实池这一腿要等到下一次 review job 落在有免密 sudo 的成员上才能验证（PR 正文也如此说明）。沙箱验证已在同一 head 上运行—— 真实场景（tmux）：N/A——纯 CI workflow 改动，无用户可见面。PR 正文中的合成工作区结果是作者自述，未在此独立复跑。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — strong engineering gated by one confirmed command-injection gap in the hardening this PR itself adds; not mergeable as-is. Stepping back: the motivation is a measured incident with a linked run, the ladder is carefully ordered, and the pinning suite is unusually disciplined — effect pins plus behavioral fixtures that catch most ways a rewrite could silently regress the heal chain. My independent proposal for this problem lands on essentially the same shape the PR implements, so there is no simpler-path objection, and the test-to-production ratio is exactly what I'd want for CI safety logic. What keeps this from approval is the single Critical, which I verified against this commit rather than taking from the review loop: the two skip-warning echoes in the same loop emit untrusted registered paths raw, contradicting the hardening contract this PR writes into the step comment and pins for the ladder's own warnings. Two lines, the PR's own idiom, plus extending the pin that already exists for the sibling case. Everything else — gate, size, scope, CI — is clean, and the round-7 deferrals are genuinely non-blocking. Verdict: request changes. Not stacking a duplicate review: the round-7 CHANGES_REQUESTED already stands on this exact commit ( 中文说明置信度：2/5 ——工程质量很高，但被本 PR 自身加固中的一处已确认命令注入缺口挡住；当前状态不可合并。 退一步看：动机是有实测、有关联 run 的事故；梯子排序审慎；pin 套件纪律性罕见地好——效果 pin 加行为夹具，能拦住改写静默退化修复链的大多数方式。我对这个问题的独立方案与 PR 的实现形态基本一致，因此没有"更简路径"的异议；测试与生产代码的比例也正是 CI 安全逻辑应有的样子。 不能批准的原因只有那条 Critical，且是针对本提交独立核实的，不是照搬评审循环：同一循环里的两处 skip 警告 echo 原样输出不可信的注册路径，与本 PR 自己写进步骤注释、并为梯子警告 pin 住的加固契约相矛盾。两行代码、用本 PR 自己的写法，再把已覆盖兄弟情形的 pin 扩展过去即可。其余——门禁、规模、范围、CI——都干净；第 7 轮延后的发现确实不构成阻断。 结论：请求修改（request changes）。 不叠加重复评审：第 7 轮的 CHANGES_REQUESTED 已经落在完全相同的提交（ — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 8a": none — all planned checks completed (~16 of ~36 tool calls)..
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.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 8a"：none — all planned checks completed (~16 of ~36 tool calls).。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(reviewCleanCode).toContain('chmod -R u+rwX'); | ||
| expect(reviewCleanCode).toContain('sudo -n chown -R'); | ||
| expect(reviewCleanCode).toContain('remove_review_tree "$leftover"'); |
There was a problem hiding this comment.
[Suggestion] The three new pins assert the ladder's mechanism strings, not its effect, and a mutation run shows the gap is real. Deleting the post-repair retry rm -rf "$abs" 2>/dev/null && return 0 keeps all three pins green — the function then repairs permissions but never retries the removal, which is exactly the warn-and-leave regression the pin comment says it exists to prevent. Deleting the non-sudo rung chmod -R u+rwX "$abs" also stays green, because the chmod -R u+rwX pin double-matches inside the sudo -n chmod -R u+rwX line — even though that rung is the only one that works on sudo-less members for self-owned read-only leftovers. Inverting the refusal comparison != → = also stays green while the root leg operates on symlink-redirected paths. (Control check: deleting the pinned remove_review_tree "$leftover" call does fail, so the harness is alive — only these three regressions slip through.) Pin the effect instead:
// The ladder must retry the removal AFTER the rungs, twice in total.
expect(reviewCleanCode.match(/rm -rf "\$abs"/g)).toHaveLength(2);
// The non-sudo rung must exist as its own command, not just inside the sudo line.
expect(reviewCleanCode).toMatch(/^\s*chmod -R u\+rwX "\$abs"/m);
// The symlink-refusal guard must survive, including the direction of its comparison.
expect(reviewCleanCode).toContain(
'refusing to repair a review worktree path that resolves through symlinks',
);
expect(reviewCleanCode).toContain('!= "$ws_real/$rel"');中文说明
这三条新断言 pin 住的是梯子的“机制字符串”，而不是它的“效果”；mutation 运行证明这个缺口是真实的。删除修复后的重试 rm -rf "$abs" 2>/dev/null && return 0 后，三条断言仍然全绿——此时函数修复了权限却从不重试删除，正是断言注释声称要防止的“告警后放任残留”回归。删除非 sudo 梯级 chmod -R u+rwX "$abs" 也依然全绿，因为 chmod -R u+rwX 断言同时匹配到了 sudo -n chmod -R u+rwX 行内——尽管该梯级是在无 sudo 成员上对自持只读残留唯一生效的一级。把拒绝判断的 != 反转为 = 也依然全绿，而 root 梯级却会对经符号链接重定向的路径执行。建议改为 pin 效果（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| rm -rf "$abs" 2>/dev/null && return 0 | ||
| echo "::warning::could not remove review worktree: $abs (permission repair failed)" | ||
| } |
There was a problem hiding this comment.
[Suggestion] The new leftover loop is the first place raw shell-glob names reach step stdout. A leftover entry whose name embeds a newline followed by ::command is echoed here un-sanitized, and the Actions runner parses ::-prefixed lines as workflow commands. A prior containerised job on this pool — exactly the actor this step exists for — can leave a name like review-pr-x$'\n::stop-commands::tok'; when the repair fails on a sudo-less member, this warning emits the raw name and the runner parses the embedded line. The reachable impact is bounded (set-env/add-path have been disabled since 2020, so this is annotation spoofing / stop-commands suppression rather than code execution), but the surface is new in this diff: pre-change, the glob rm -rf … || true never echoed leftover names, and the worktree-list loop's values come from git porcelain, which C-escapes newlines. Strip newlines before echoing:
| rm -rf "$abs" 2>/dev/null && return 0 | |
| echo "::warning::could not remove review worktree: $abs (permission repair failed)" | |
| } | |
| rm -rf "$abs" 2>/dev/null && return 0 | |
| echo "::warning::could not remove review worktree: ${abs//$'\n'/ } (permission repair failed)" | |
| } |
中文说明
新增的残留目录循环是 shell glob 原始文件名第一次直接进入步骤 stdout。名字中嵌入换行加 ::command 的残留条目在这里会未经净化地被 echo 出来，而 Actions runner 会把以 :: 开头的行解析为 workflow 命令。池上先前的容器化 job（正是本步骤要处理的角色）可以留下形如 review-pr-x$'\n::stop-commands::tok' 的名字；当修复在无 sudo 成员上失败时，这条 warning 会原样输出该名字，runner 随即解析其中嵌入的命令行。可达影响有上限（set-env/add-path 自 2020 年已禁用，因此只能伪造 annotation / 触发 stop-commands 抑制，而非代码执行），但该注入面是本 diff 新引入的：改动前 glob 的 rm -rf … || true 从不 echo 残留名字，而 worktree 列表循环的值来自 git porcelain（会对换行做 C 转义）。建议在 echo 前剥离换行（见上方 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| # repair ladder instead: chmod what this user owns, then passwordless | ||
| # sudo chown/chmod where the pool member has it, each followed by a | ||
| # retry. Members without sudo still degrade to a named warning — |
There was a problem hiding this comment.
[Suggestion] The comment says each rung is "each followed by a retry", but remove_review_tree retries rm -rf exactly once, after ALL rungs — there is no retry between the chmod rung and the sudo rung, so the root leg escalates even when chmod already repaired the tree. This file is deliberately comment-driven and its cleanup recipe is contract-pinned by tests, so a maintainer auditing when the root leg fires will conclude a chmod-repaired tree is removed before sudo is attempted — the opposite of what runs. Reword the comment (or insert rm -rf "$abs" 2>/dev/null && return 0 between the two rungs to make the code match, which also skips the root leg when chmod sufficed):
| # repair ladder instead: chmod what this user owns, then passwordless | |
| # sudo chown/chmod where the pool member has it, each followed by a | |
| # retry. Members without sudo still degrade to a named warning — | |
| # repair ladder instead: chmod what this user owns, then passwordless | |
| # sudo chown/chmod where the pool member has it, with a single retry | |
| # after the ladder. Members without sudo still degrade to a named warning — |
中文说明
注释称每个梯级都“各跟一次重试”，但 remove_review_tree 只在所有梯级之后重试一次 rm -rf——chmod 梯级与 sudo 梯级之间并没有重试，因此即使 chmod 已经修好了树，root 梯级仍会升级执行。该文件刻意以注释驱动，且其清理流程被契约测试 pin 住，审计 root 梯级触发时机的维护者会得出“chmod 修好的树会在动用 sudo 之前被删除”的结论——与实际行为相反。建议改写注释（或在两个梯级之间插入 rm -rf "$abs" 2>/dev/null && return 0 使代码与注释一致，这样 chmod 已修复时还能跳过 root 梯级）（见上方 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| sudo -n chmod -R u+rwX "$abs" 2>/dev/null || true | ||
| fi | ||
| rm -rf "$abs" 2>/dev/null && return 0 | ||
| echo "::warning::could not remove review worktree: $abs (permission repair failed)" |
There was a problem hiding this comment.
[Suggestion] The ladder's terminal warning carries none of the state needed to act on it: every rm/chmod/chown stderr is discarded via 2>/dev/null, and the message does not say whether passwordless sudo was probed, who owns the survivor, or what the final rm reported. The refusal warning one branch up likewise hard-codes "resolves through symlinks" even when the actual trigger was a non-workspace path or a missing/failed realpath. This step exists because a leftover tree paged someone (run 32577821716); when the warning fires, the on-call cannot distinguish no-passwordless-sudo (expected degrade — fix the pool), sudo-ran-but-chown-denied (sudoers drift), foreign-owned parent directory (no tree repair can fix the unlink), or a non-permission cause mislabelled as "permission repair failed" — each needs a different fix, and today the operator must SSH into the member and reconstruct ownership by hand. Enrich both warnings with the deciding state, e.g.:
sudo_probe=absent
if command -v sudo >/dev/null 2>&1 && sudo -n true 2>/dev/null; then sudo_probe=ok; fi
# …in the failure warning:
echo "::warning::could not remove review worktree: $abs (permission repair failed; sudo probe: $sudo_probe; owner: $(ls -ld "$abs" 2>/dev/null | awk '{print $3}'))"(Note: combine with the newline sanitization suggested on the adjacent thread.)
中文说明
梯子最终的 warning 不带任何可供处置的状态：所有 rm/chmod/chown 的 stderr 都被 2>/dev/null 丢弃，消息既没说明是否探测到免密 sudo、残留目录属于谁，也没说最后一次 rm 报了什么；上方拒绝分支的 warning 同样写死“经由符号链接解析”，即便真实触发原因是路径不在工作区之下或 realpath 缺失/失败。这个步骤的存在本身就源于残留树曾经 page 过人（run 32577821716）；warning 触发时，值班人无法从日志区分：没有免密 sudo（预期降级——修池子）、sudo 执行但 chown 被拒（sudoers 漂移）、父目录属主是他人（任何树内修复都无法解决 unlink）、或被误标为“权限修复失败”的非权限原因——每种都需要不同的处置，而现在只能 SSH 上成员机手工重建属主信息。建议在两条 warning 中补充决定性状态（示例见上方代码块，并与相邻线程建议的换行净化合并使用）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| echo "::warning::could not remove review worktree: $abs (permission repair failed)" | ||
| } |
There was a problem hiding this comment.
[Suggestion] remove_review_tree's only non-return 0 exit path hands back the bare final echo's status, and the new leftover loop calls the function unwrapped — no || true after done, unlike the two piped loops. The step runs under Actions' default bash -eo pipefail, and the step's own set -uo pipefail does not clear the inherited -e (verified by control experiment), so a failed write to stdout — e.g. the runner log pipe dying during teardown while this if: always() step still runs — yields a nonzero function return, fires errexit, and reds the job, against the contract this diff itself states ("the heal chain must never fail the job"). The trigger is rare today, but the same grammar silently turns any future edit that makes the tail statement fallible into a job-killer. Make the contract unconditional:
| echo "::warning::could not remove review worktree: $abs (permission repair failed)" | |
| } | |
| echo "::warning::could not remove review worktree: $abs (permission repair failed)" | |
| return 0 | |
| } |
中文说明
remove_review_tree 唯一不以 return 0 结束的退出路径会把最后那条裸 echo 的状态作为函数返回值，而新增的残留循环对函数的调用没有包裹——done 之后没有 || true（两个管道循环都有）。该步骤运行在 Actions 默认的 bash -eo pipefail 之下，步骤自身的 set -uo pipefail 并不会清除继承来的 -e（已用对照实验验证）；因此一次 stdout 写失败——例如这个 if: always() 步骤仍在运行时 runner 日志管道在 job 拆除阶段死掉——就会产生非零函数返回，触发 errexit，把 job 染红，违背本 diff 自己声明的契约（“自愈链永远不能让 job 失败”）。今天这个触发很罕见，但同样的语法会让未来任何把尾部语句变成可失败的改动悄悄成为 job 杀手。建议把契约变成无条件成立（见上方 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…arnings Review feedback on the permission-repair ladder: - Pin the ladder's effect in the contract test (three removal attempts, isolated non-sudo chmod rung, refusal-comparison direction) — the old mechanism substrings stayed green when the post-repair retry was deleted, when the non-sudo rung was deleted, and when the refusal comparison was inverted (all reproduced by mutation before the fix). - Retry the removal after the chmod rung so a chmod-repaired tree never escalates to passwordless sudo; the step comment's "each followed by a retry" is now literally true. - Strip newlines from leftover paths before echoing: leftover names are untrusted glob entries, and a fresh line on the runner's stdout would parse as a workflow command. - Both warnings now carry the deciding state: the refusal names the branch that fired; the failure warning reports the sudo probe result and the survivor's owner. - Return 0 unconditionally so a failed warning echo can never fail the if: always() job via errexit.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9748 (addressing 5 automated-reviewer Suggestions, review round 1)All five inline findings are addressed and resolved in code (commit Findings and decisions
Additionally: the new owner-extraction [ic:5381949363] (workflow size baseline note) — informational; this round's size delta follows the same record-the-shipped-size convention. Mutation probes (each probe mutates the workflow, expects the focused test RED, then restores)
Functional smoke (non-root uid 1000, no passwordless sudo — exactly the degraded actor)
Verification
中文说明本轮摘要 — PR #9748（处理自动化审查者的 5 条 Suggestion，审查第 1 轮）五条行内发现全部已在代码中处理并解决（提交 发现与处置
另外：新增的属主提取 [ic:5381949363]（workflow 体积基线说明）— 信息性评论；本轮的体积变化沿用同样的“记录实际发布体积”约定。 Mutation 探针（每个探针对 workflow 做变异、期望聚焦测试变红、然后还原）
功能冒烟（非 root uid 1000、无免密 sudo — 恰是降级当事环境）
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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.
中文说明
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| sudo -n chmod -R u+rwX "$abs" 2>/dev/null || true | ||
| fi | ||
| rm -rf "$abs" 2>/dev/null && return 0 | ||
| echo "::warning::could not remove review worktree: ${abs//$'\n'/ } (permission repair failed; sudo: $sudo_probe; owner: $(ls -ld "$abs" 2>/dev/null | awk '{print $3}'))" |
There was a problem hiding this comment.
[Critical] The newline sanitization this round added covers the two direct ${abs} interpolations only, and it is LF-only — two demonstrated entrances still inject workflow commands into this job's stdout, the exact class the step comment and the contract test claim to have closed. First, the owner: enrichment on this line embeds $(ls -ld "$abs" | awk '{print $3}'): GNU ls piped to a non-tty prints a newline-bearing path raw across lines, and awk prints field 3 of every line, so a leftover named review-pr-x$'\n'a b ::stop-commands::tok emits a standalone ::stop-commands::tok line the runner parses as a workflow command. Second, ${abs//$'\n'/ } strips LF only, and the Actions runner splits step stdout on bare CR (actions/runner ProcessInvoker.cs reads via StreamReader.ReadLine(), which terminates on \r; OutputManager.cs hands every :: line to the command parser), so a CR-bearing name injects a second command line through both warnings. Reachable impact is bounded (set-env/add-path are disabled runner-side — this is annotation forgery / ::stop-commands:: / ::add-mask:: on the privileged review job, not code execution), but the poison tree survives the sweep, so every later job on the runner re-emits it, and the existing ${abs//$'\n'/ } x2 pin passes throughout because it cannot see inside $(…).
Witness — probe of the extracted step against a root-owned leftover named review-pr-x$'\n'a b ::stop-commands::tok\nc d e on a password-gated-sudo host:
line 2: ::warning::could not remove review worktree: …review-pr-x a b ::stop-commands::tok c d e (permission repair failed; sudo: absent; owner: root
line 3: ::stop-commands::tok
line 4: e)
a standalone, runner-parseable command line; with the fix below applied the same probe printed a single line — zero injected commands.
| echo "::warning::could not remove review worktree: ${abs//$'\n'/ } (permission repair failed; sudo: $sudo_probe; owner: $(ls -ld "$abs" 2>/dev/null | awk '{print $3}'))" | |
| echo "::warning::could not remove review worktree: ${abs//[$'\r\n']/ } (permission repair failed; sudo: $sudo_probe; owner: $(ls -ld "$abs" 2>/dev/null | awk 'NR==1 {print $3}'))" |
Apply the same [$'\r\n'] strip to the refusal warning above (line ~1911), and extend the contract pin to cover every expansion in both warnings rather than only the two ${abs} occurrences.
中文说明
本轮新增的换行净化只覆盖两个直接的 ${abs} 插值，且只剥 LF——仍有两条已被证明的入口可以向本 job 的 stdout 注入 workflow 命令，正是步骤注释与契约测试声称已关闭的那一类。其一，本行的 owner: 增强内嵌 $(ls -ld "$abs" | awk '{print $3}')：GNU ls 管道输出时会把带换行的路径原样跨行打印，awk 会打印每一行的第 3 个字段，因此名为 review-pr-x$'\n'a b ::stop-commands::tok 的残留会输出一条独立的 ::stop-commands::tok 行，被 runner 当作 workflow 命令解析。其二，${abs//$'\n'/ } 只剥 LF，而 Actions runner 按裸 CR 切分步骤 stdout（actions/runner 的 ProcessInvoker.cs 用 StreamReader.ReadLine() 读取，\r 即行终止符；OutputManager.cs 会把任何含 :: 的行交给命令解析器），因此带 CR 的名字可经由两条 warning 注入第二条命令行。可达影响有上限（set-env/add-path 已在 runner 侧禁用——这是对特权 review job 的 annotation 伪造 / ::stop-commands:: / ::add-mask::，而非代码执行），但毒树在清扫后依然存活，该 runner 上之后的每个 job 都会再次触发；现有的 ${abs//$'\n'/ } x2 断言全程保持绿色，因为它看不到 $(…) 内部。
critical 证据：对提取出的步骤做 probe——root 属主、名字为 review-pr-x$'\n'a b ::stop-commands::tok\nc d e 的残留，在 sudo 需密码的主机上输出了独立的 ::stop-commands::tok 行；应用下方修复后同一 probe 只输出单行，注入命令数为零。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const reviewCleanCode = stripComments(reviewCleanStep); | ||
| // Three removal attempts: the initial rm plus one retry after EACH | ||
| // repair rung, so a chmod-repaired tree never escalates to sudo. | ||
| expect(reviewCleanCode.match(/rm -rf "\$abs"/g)).toHaveLength(3); |
There was a problem hiding this comment.
[Suggestion] The nine new pins assert presence and counts of the ladder's text, but not its order or effect — and mutation runs against the real suite show the gap is real. Three behavior-breaking mutations keep all 5 tests green: setting rel='' (every leftover then refuses as "outside the workspace", no rung ever runs, and the incident this PR exists for recurs); relocating the refusal guard BELOW the chmod/sudo rungs (verified in a mirror tree — chmod -R follows a symlink operand, so the privileged rungs could then operate through a planted link); and swapping the sudo block above the chmod rung (breaking the "chmod-repaired tree never escalates to sudo" property the test comment claims). Controls confirm the harness is alive: dropping the sudo rung, inverting !=, or deleting one newline-strip all go red. The pins catch some rewrites — just not order or effect. This supersedes round-1 R1-1: text pins cannot enumerate the rewrite space, and an ordering assertion closes the demonstrated shapes structurally.
Witness — mutation arms in a freshly reset scratch tree, real vitest suite: baseline 5/5 green; rel='' → 5/5 green; guard relocated below the rungs → 5/5 green; control (sudo rung dropped) → 1 test failed.
| expect(reviewCleanCode.match(/rm -rf "\$abs"/g)).toHaveLength(3); | |
| expect(reviewCleanCode.match(/rm -rf "\$abs"/g)).toHaveLength(3); | |
| // Order matters as much as presence: the refusal guard must precede the | |
| // repair rungs, or a rewrite can chmod/chown through a planted link | |
| // before the check ever runs. | |
| const guardPos = reviewCleanCode.indexOf('if [ -n "$reason" ]'); | |
| expect(guardPos).toBeGreaterThan(-1); | |
| expect(guardPos).toBeLessThan(reviewCleanCode.indexOf('chmod -R u+rwX "$abs"')); | |
| expect(guardPos).toBeLessThan(reviewCleanCode.indexOf('sudo -n chown -R')); |
Stronger still: extract the function from the step text and execute it against a tmp fixture (a self-owned read-only tree removed by the chmod rung — skipped when running as root, where CAP_DAC_OVERRIDE makes the fixture vacuous; a planted symlink refused with the target untouched; a newline-bearing name yielding a single-line warning), capability-gated like the existing awkAvailable pattern.
中文说明
这九条新断言 pin 住的是梯子文本的"存在与计数"，而不是其"顺序或效果"——对真实套件做的 mutation 运行证明缺口是真实的。三种破坏行为的变异都让 5 条测试全绿：把 rel=''（此后每个残留都会被当作"在工作区之外"拒绝，任何梯级都不会执行，本 PR 要解决的事故会复发）；把拒绝守卫移到 chmod/sudo 梯级之下（已在镜像树中验证——chmod -R 会跟随符号链接操作数，特权梯级就可能顺着植入的链接操作）；把 sudo 块换到 chmod 梯级之前（破坏测试注释所声称的"chmod 修好的树不会升级到 sudo"属性）。对照实验证明测试框架是活的：删掉 sudo 梯级、反转 !=、删除任一新换行剥离都会变红。这些断言能抓住一部分改写——只是抓不住顺序与效果。本条取代第 1 轮的 R1-1：文本断言无法枚举改写空间，顺序断言能结构性地关闭已证明的变异形状。
证据：在全新重置的临时树中对真实 vitest 套件做 mutation——基线 5/5 绿；rel='' → 5/5 绿；守卫移到梯级之下 → 5/5 绿；对照（删掉 sudo 梯级）→ 1 条测试失败。更彻底的方案是把函数从步骤文本中提取出来、对 tmp 夹具真实执行（自持只读树经 chmod 梯级被删除——以 root 运行时跳过，CAP_DAC_OVERRIDE 会使夹具失效；植入的符号链接被拒绝且目标不受影响；带换行的名字只产生单行 warning），并按现有 awkAvailable 模式做能力门控。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| fi | ||
| chmod -R u+rwX "$abs" 2>/dev/null || true | ||
| rm -rf "$abs" 2>/dev/null && return 0 | ||
| local sudo_probe='absent' |
There was a problem hiding this comment.
[Suggestion] The failure warning's sudo: diagnostic conflates two states this change is specifically about: "sudo not installed" and "sudo installed but password-gated". On a member with sudo present but no NOPASSWD entry, command -v sudo succeeds while sudo -n true fails, so sudo_probe stays absent and the warning prints sudo: absent — sending the on-call to investigate a missing package when the remediation is a sudoers rule. The incident this step exists for (run 32577821716) was exactly the gated state: "the runner had no passwordless sudo". Corroborated empirically: a probe host with password-gated sudo printed sudo: absent from the unmodified step. Split the probe into three states; the existing sudo: \$sudo_probe test pin survives unchanged.
| local sudo_probe='absent' | |
| local sudo_probe='password-gated' | |
| command -v sudo >/dev/null 2>&1 || sudo_probe='absent' |
中文说明
失败 warning 里的 sudo: 诊断把两种本改动恰好要区分的状态混为一谈："未安装 sudo" 与 "安装了 sudo 但需要密码"。在有 sudo 但没有 NOPASSWD 条目的成员上，command -v sudo 成功而 sudo -n true 失败，sudo_probe 保持 absent，warning 输出 sudo: absent——值班人会去排查缺包，而正确的处置是配置 sudoers 规则。本步骤所针对的事故（run 32577821716）恰好就是"有 sudo 但需密码"状态："the runner had no passwordless sudo"。实测佐证：在一台 sudo 需密码的主机上，未改动的步骤打印了 sudo: absent。把探测拆成三态即可；现有的 sudo: \$sudo_probe 测试断言不受影响。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1]; | ||
| // The step's owner-extraction awk is not a worktree filter: anchor | ||
| // on the filter's shape, not the first awk in the step. | ||
| const filter = reviewCleanStep.match(/awk '(\$1 == "worktree"[^']+)'/)?.[1]; |
There was a problem hiding this comment.
[Suggestion] This diff pastes the same filter-extraction regex plus the identical two-line rationale comment into two tests (here and in the behavioral filter test below) instead of deriving it once — even though this file's own convention hoists exactly this kind of shared derivation from the parsed YAML to module scope (worktreePrefix, branchFamily, reviewCleanStep). The coupling is not hypothetical: this round already had to edit both copies in lockstep, and a future filter change that updates only one leaves the pin test pinning one filter while spawnSync('awk', [filter]) executes a different one — the semantic check no longer validates what the pinning test pins, and the drift surfaces later as a confusing failure elsewhere. Hoist one module-level derivation next to reviewCleanStep and reference it in both tests:
// next to reviewCleanStep (rationale comment kept once, here)
const worktreeFilter = reviewCleanStep.match(
/awk '(\$1 == "worktree"[^']+)'/,
)?.[1];
// in both tests:
const filter = worktreeFilter;中文说明
本 diff 把同一个过滤器提取正则和完全相同的两行注释复制进了两个测试（此处与下方的行为过滤器测试），而不是只推导一次——尽管本文件的既有惯例就是把这类从解析出的 YAML 推导的共享量提升到模块作用域（worktreePrefix、branchFamily、reviewCleanStep）。这种耦合不是假设：本轮就已经被迫同步修改了两处副本；未来任何只改其中一处的过滤器变更，都会让 pin 测试钉住一个过滤器、而 spawnSync('awk', [filter]) 执行另一个——语义检查不再验证 pin 测试所钉住的东西，偏差会在别处以令人困惑的失败形式出现。建议在 reviewCleanStep 旁提升一个模块级推导，两个测试都引用它（注释只保留一份）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…ree warnings (#9748) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #9748 (round: post
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 3c": run scripts/tests/review-worktree-cleanup-workflow.test.js via npm run test:scripts — worktree has no node_modules and a from-scratch npm ci (with prepare ….
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
.github/workflows/qwen-code-pr-review.yml:1962 — [probe] a registered worktree the ladder already failed on is re-laddered identically by the leftover loop (doubled repair cost + duplicate warning)scripts/tests/review-worktree-cleanup-workflow.test.js:206 — [probe] contract pins cover remove_review_tree "$leftover" but not the twin "$worktree" fallback arm — reverting the arm to the old unguarded echo ships green.github/workflows/qwen-code-pr-review.yml:1885 — [probe] remove_review_tree's first rm -rf precedes the containment guard and never validates $1 (empty-arg rm -rf of the workspace root; latent — no current caller can pass one)
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: scripts/tests/review-worktree-cleanup-workflow.test.js (findings in round 2; 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. 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.)
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 3c"：run scripts/tests/review-worktree-cleanup-workflow.test.js via npm run test:scripts — worktree has no node_modules and a from-scratch npm ci (with prepare …。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：scripts/tests/review-worktree-cleanup-workflow.test.js（第 2 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const fnStart = reviewCleanCode.indexOf('remove_review_tree() {'); | ||
| const reviewTreeFn = reviewCleanCode.slice( | ||
| fnStart, | ||
| reviewCleanCode.indexOf('\n}\n', fnStart), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The 40-line remove_review_tree bash function — including this round's CR/LF-strip and NR==1 owner-extraction hardening — is covered only by static substring/order pins; nothing ever parses or executes it. Two mutation shapes survive every pin in this file yet break the step at runtime, which falsifies the round-2 rationale for declining an execution harness (the R2-2 disposition argued the ordering + derivation pins structurally close every demonstrated mutation shape):
- Break the unpinned existence check (
[ -e "$abs" || [ -L "$abs" ] || return 0, missing]): the suite stays green, and the extracted function then prints[: missing ']', returns 0, and silently no-ops on every existing dir — the ladder never removes anything. - Drop the closing quote of the refusal
echo: the suite stays green, butbash -non the whole step fails with a syntax error — theif: always()cleanup step dies before any prune/remove/sweep runs, on every review job.
Witness (probe at HEAD 7a9fe5d4fa): mutant 1 → suite 5/5 green, runtime [: missing ']', rc=0, tree survived (the same probe removes it at HEAD); mutant 2 → suite 5/5 green, bash -n → syntax error near unexpected token.
This is a reconsideration with new evidence, not a re-demand of the declined harness: if the harness stays declined, the demonstrated shapes also admit narrower pins (exact existence-check spelling, refusal-echo quote integrity) — note bash -n alone catches the quote-drop but not the missing-] no-op, which parses fine.
// sketch: execute the already-extracted function (the file's awk test is precedent;
// append the closing `}` the slice drops) against a fixture GITHUB_WORKSPACE:
// (1) a plain leftover dir is actually removed;
// (2) a symlinked leftover is refused without touching its target;
// (3) a leftover named $'\n::error::injected' yields exactly one `::warning::` line中文说明
40 行的 remove_review_tree bash 函数（包括本轮的 CR/LF 剥离与 NR==1 owner 提取加固）目前只有静态子串/顺序断言覆盖，从未被解析或执行。有两种 mutation 形态能穿过本文件所有断言、却在运行时破坏该步骤，这证伪了第 2 轮拒绝执行验证框架时的理由（R2-2 处置认为顺序与派生断言已在结构上封闭了所有已演示的 mutation 形态）：
- 破坏未被断言的存在性检查（
[ -e "$abs" || [ -L "$abs" ] || return 0，缺]）：测试套件仍然全绿，而提取出的函数会打印[: missing ']'、返回 0、对每个存在的目录静默 no-op——梯子什么都不会删除。 - 删掉拒绝分支
echo的闭合引号：测试套件仍然全绿，但bash -n对整个步骤报语法错误——if: always()清理步骤在任何 prune/删除/清扫运行前就死掉，每个 review job 都会如此。
critical 证据（在 HEAD 7a9fe5d4fa 上的 probe）：mutant 1 → 套件 5/5 绿，运行时 [: missing ']'，rc=0，树残留（同一 probe 在 HEAD 下会删除它）；mutant 2 → 套件 5/5 绿，bash -n → syntax error near unexpected token。
这是带有新证据的重新考虑，不是重新强推已被拒绝的框架：如果仍拒绝框架，上述两种已演示的形态也可以用更窄的断言封闭（存在性检查的精确拼写、拒绝分支 echo 的引号完整性）——注意 bash -n 本身能抓到引号缺失，但抓不到缺 ] 的 no-op（它能正常通过解析）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(reviewCleanCode).toContain( | ||
| "command -v sudo >/dev/null 2>&1 || sudo_probe='absent'", | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The new probe-state block pins two of the three sudo_probe states ('password-gated' default, 'absent' assignment) but not the sudo_probe='ok' assignment, even though the block's own comment makes probe-state discrimination the point; deleting the 'ok' line survives the entire suite — mutation-probed: 5/5 green, and adding the pin below flips the mutant red while HEAD + pin stays green. The runtime misdirection is demonstrated too: with a PATH-shim sudo that exits 0 and a foreign-owned tree that still defeats rm, HEAD warns sudo: ok; owner: 12345 while the mutant warns sudo: password-gated; owner: 12345 — sending the on-call to write a sudoers rule that already exists instead of investigating the filesystem, exactly the misdirection class the adjacent comment cites for the other two states.
| expect(reviewCleanCode).toContain( | |
| "command -v sudo >/dev/null 2>&1 || sudo_probe='absent'", | |
| ); | |
| expect(reviewCleanCode).toContain( | |
| "command -v sudo >/dev/null 2>&1 || sudo_probe='absent'", | |
| ); | |
| expect(reviewCleanCode).toContain("sudo_probe='ok'"); |
中文说明
新增的探针状态断言块固定了 sudo_probe 三种状态中的两种（'password-gated' 默认值、'absent' 赋值），却没有固定 sudo_probe='ok' 赋值，尽管该块自己的注释明确以区分探针状态为目的；删除 'ok' 行后整个套件仍然全绿——已做 mutation 验证：5/5 绿，加入下方断言后 mutant 变红，HEAD + 该断言保持绿。运行时误导也已演示：用退出码为 0 的 PATH 垫片 sudo 加一棵外部属主、仍无法删除的树，HEAD 的 warning 输出 sudo: ok; owner: 12345，而 mutant 输出 sudo: password-gated; owner: 12345——这会把 on-call 引去写一条已存在的 sudoers 规则，而不是排查文件系统，正是相邻注释为另外两种状态所引用的误导类型。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #9748 (round 4)Both inline findings were verified by reproduction first, then addressed in one commit ( Feedback points and dispositions[rc:3837352569] [Suggestion] Reproduced the claim on the pre-round tree before changing anything:
Both mutants confirmed, so the round-2 rationale for declining execution coverage is falsified. Fix: execute the extracted function against fixture workspaces (precedent: the file's existing awk-execution test), with three behavioral cases:
Fixtures are skipped where POSIX permission semantics are unavailable ( [rc:3837352575] [Suggestion] missing Reproduced first: deleting the Mutation probes on the new guards (pre-commit)
Notes
VerificationCommands actually run, in order:
Known-unrelated observation: one earlier 中文说明Autofix 轮次总结 — PR #9748（第 4 轮）两条行内发现均先经复现验证，然后在一次提交中处理完毕（ 反馈要点与处置[rc:3837352569] [Suggestion] 在改动任何代码之前，先在轮次前的代码树上复现了该主张：
两个 mutant 均被证实，因此第 2 轮拒绝执行覆盖的理由被证伪。修复：将提取出的函数在夹具工作区上实际执行（先例：本文件已有的 awk 执行测试），包含三个行为用例：
在 POSIX 权限语义不可用的环境（缺 [rc:3837352575] [Suggestion] 缺少 先复现：删除 新守卫的 mutation 探针（提交前）
说明
验证实际执行的命令，按顺序：
已知无关观察：较早的一次 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-11 duplicate
remove_review_treeextraction in the pinning test (divergent end boundaries vs the module-scope constant) — overlaps existing comment 3837352569 at scripts/tests/review-worktree-cleanup-workflow.test.js:275
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 10.
Convergence: round 4 posted 13 inline comment(s), 13 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: scripts/tests/review-worktree-cleanup-workflow.test.js (findings in round 3; 13 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, 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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 10 轮的反审轮数上限内未收敛。
收敛情况：第 4 轮发布了 13 条行内评论，其中 13 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：scripts/tests/review-worktree-cleanup-workflow.test.js（第 3 轮已出过发现，本轮又有 13 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const permissionFixturesAvailable = | ||
| bashAvailable && process.platform !== 'win32' && process.geteuid?.() !== 0; |
There was a problem hiding this comment.
[Critical] The fixture gate probes bash/Windows/root, but not realpath, so the new behavioral tests reach the merge_group-only test_macos lane and go red there. macos-latest currently resolves to macOS 26 arm64, and that runner image ships no realpath(1) — no coreutils in the image inventory (checked live against the actions/runner-images readme and toolset JSON; Homebrew coreutils would expose only grealpath without a gnubin PATH entry, which the image never adds). On that lane the gate opens (bash exists, non-Windows, non-root), and in this test realpath -- fails, so the function falls back to abs_real='' and emits the path could not be resolved refusal warning instead of the asserted could not remove review worktree prefix. The lane is skipped on PRs, so the first red lands in the merge queue — ejecting the entry and stalling every PR batched with it.
Witness (no-realpath PATH model of the lane, suite unmodified):
Tests 1 failed | 7 passed — sole failure at this test's assertion
baseline with realpath present: Tests 8 passed (8)
Suggested fix — probe the capability the way the sibling suites do (hasGnuRealpath in serve-ab-workflow.test.js):
const realpathAvailable =
spawnSync('realpath', ['--', '/'], { stdio: 'ignore' }).status === 0;and AND it into permissionFixturesAvailable, or it.skipIf only the third test so tests 1-2 keep running where resolution is unavailable.
中文说明
夹具门控探测了 bash/Windows/root，但没有探测 realpath，因此新的行为测试会到达仅 merge_group 触发的 test_macos 通道并在那里变红。macos-latest 目前解析为 macOS 26 arm64，该 runner 镜像不提供 realpath(1)——镜像清单中没有 coreutils（已实时核对 actions/runner-images 的 readme 与 toolset JSON；Homebrew coreutils 在不添加 gnubin PATH 条目时只暴露 grealpath，而镜像并未添加）。在该通道上门控会打开（bash 存在、非 Windows、非 root），本测试中 realpath -- 失败，函数回退到 abs_real=''，输出 path could not be resolved 的拒绝 warning，而非断言要求的 could not remove review worktree 前缀。由于该通道在 PR 页面被跳过，首次变红发生在合并队列——会踢出队列条目，阻塞与之同批的所有 PR。
证据（对该通道做去除 realpath 的 PATH 建模，套件未改动）：
Tests 1 failed | 7 passed — 唯一失败就在本测试的断言
含 realpath 的基线：Tests 8 passed (8)
建议修复——像姊妹套件那样探测该能力（serve-ab-workflow.test.js 的 hasGnuRealpath）：
const realpathAvailable =
spawnSync('realpath', ['--', '/'], { stdio: 'ignore' }).status === 0;然后与 permissionFixturesAvailable 取与；或只对第三个测试 it.skipIf，让测试 1-2 在缺少该能力的平台上继续运行。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| '-c', | ||
| `set -uo pipefail\n${removeReviewTreeFn}\nremove_review_tree "$@"`, |
There was a problem hiding this comment.
[Suggestion] The extract-and-execute harness runs remove_review_tree under set -uo pipefail, omitting the -e the production runner applies (bash --noprofile --norc -eo pipefail); the step's own first line does not turn errexit back off. The sibling harnesses in this directory mirror the runner's -e, and this file's own comment says steps run under bash -e + pipefail. Concretely: an injected unguarded failing command stays invisible under the shipped harness (8/8 green) while set -euo pipefail fails three behavioral tests on it — so a future edit dropping one of the ladder's || true guards keeps these tests green while errexit kills the if: always() cleanup step in production, the exact job failure this suite pins against elsewhere.
Witness (same mutant, both arms):
shipped harness (set -uo pipefail) + mutant: Tests 8 passed (8) — mutant invisible
set -euo pipefail harness + same mutant: 3 behavioral tests fail
set -euo pipefail harness + clean code: Tests 8 passed (8)
Suggested fix:
`set -euo pipefail\n${removeReviewTreeFn}\nremove_review_tree "$@"`— safe for the current function: every failing command in it is already errexit-exempt or guarded, so all fixtures stay green.
中文说明
提取-执行测试框架在 set -uo pipefail 下运行 remove_review_tree，漏掉了生产环境 runner 所带的 -e（bash --noprofile --norc -eo pipefail）；步骤自身的首行并不会重新关闭 errexit。同目录下的姊妹框架都镜像了 runner 的 -e，本文件自己的注释也写明步骤运行于 bash -e + pipefail 之下。具体后果：注入一条未加防护的失败命令，在现有框架下不可见（8/8 全绿），而 set -euo pipefail 会让三个行为测试失败——因此未来若删除梯子中某个 || true 防护，这些测试仍为绿，生产环境的 errexit 却会杀死 if: always() 清理步骤，正是本套件在别处明确钉住的 job 失败形态。
证据（同一变异，双臂对照）：
现有框架（set -uo pipefail）+ 变异：Tests 8 passed (8) —— 变异不可见
set -euo pipefail 框架 + 同一变异：3 个行为测试失败
set -euo pipefail 框架 + 干净代码：Tests 8 passed (8)
建议修复：把框架脚本改为 set -euo pipefail。对当前函数是安全的：其中每个失败命令都已豁免 errexit 或加了防护，所有夹具仍为绿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(reviewCleanCode.match(/rm -rf "\$abs"/g)).toHaveLength(3); | ||
| // The non-sudo rung must exist as its own command, not just inside the | ||
| // sudo line. | ||
| expect(reviewCleanCode).toMatch(/^\s*chmod -R u\+rwX "\$abs"/m); |
There was a problem hiding this comment.
[Suggestion] The rungs' 2>/dev/null || true failure guards are pinned by nothing, and no fixture can make a rung fail (fixture trees are user-owned, so chmod succeeds; root hosts are gated out) — dropping || true from the chmod rung ships 8/8 green, and even a -e-hardened harness cannot catch it (probe: status=0 under both harnesses, since the fixture's chmod never fails). That matters because the leftover for-loop calls remove_review_tree bare under the runner's bash -eo pipefail — NOT exempt from errexit, unlike the piped worktree-list loop's done || true. In production, on a foreign-owned leftover chmod exits EACCES, errexit kills the cleanup step mid-ladder, and the trailing second worktree prune, branch-deletion loop, and lease rm -f never run — the if: always() step goes red and branches/leases leak on the runner.
Witness:
mutant (guard dropped) + shipped suite: Tests 8 passed (8)
errexit probe under the runner's flags: bare call site exit=1 (tail never runs); piped site exit=0 (absorbed)
suggested pin: red on mutant, green on clean
Suggested fix — pin the guarded form, and extend the sudo pins to carry their guards too:
expect(reviewCleanCode).toMatch(/^\s*chmod -R u\+rwX "\$abs" 2>\/dev\/null \|\| true$/m);中文说明
各梯级的 2>/dev/null || true 失败防护没有被任何断言钉住，也没有任何夹具能让梯级失败（夹具树为当前用户所有，chmod 必然成功；root 主机被门控排除）——删掉 chmod 梯级的 || true 后 8/8 全绿上线，即便换上带 -e 的框架也抓不到（探针：两种框架下 status=0，因为夹具的 chmod 从不失败）。这很要紧：残留 for 循环在 runner 的 bash -eo pipefail 下裸调用 remove_review_tree——不像带 done || true 的 worktree 列表管道循环，它并不豁免 errexit。生产中，遇到他人属主的残留时 chmod 以 EACCES 退出，errexit 会在梯子中途杀死清理步骤，其后的第二次 worktree prune、分支删除循环和租约 rm -f 都不会执行——if: always() 步骤变红，分支/租约泄漏在 runner 上。
证据：变异（删防护）+ 现有套件 8/8 全绿；按 runner 标志做 errexit 探针：裸调用点 exit=1（尾部未执行），管道调用点 exit=0（被吸收）；建议断言在变异下红、干净代码下绿。
建议修复——钉住带防护的形式（并把 sudo 断言扩展为同样带上防护）：
expect(reviewCleanCode).toMatch(/^\s*chmod -R u\+rwX "\$abs" 2>\/dev\/null \|\| true$/m);— qwen3.8-max via Qwen Code /review (v0.22.0)
| // The non-sudo rung must exist as its own command, not just inside the | ||
| // sudo line. | ||
| expect(reviewCleanCode).toMatch(/^\s*chmod -R u\+rwX "\$abs"/m); | ||
| expect(reviewCleanCode).toContain('sudo -n chown -R'); |
There was a problem hiding this comment.
[Suggestion] The sudo rung is pinned only by the verb substring sudo -n chown -R; two mutants ship 8/8 green. (1) Its second half sudo -n chmod -R u+rwX "$abs" can be deleted outright: no pin mentions sudo -n chmod (the chmod regex/order pins match only the non-sudo rung), and all fixtures chmod-555 the PARENT, so none observes the rung even on a passwordless-sudo host. (2) The chown ownership target "$(id -u):$(id -g)" can be mutated to root:root: nothing captures id -u/id -g, and no fixture asserts the warning's owner: field. Either way, on a pool member WITH passwordless sudo — the member the rung exists to heal — a foreign-owned leftover with a mode-locked inner directory is chowned back but never made writable (or actively locked to root), the runner-user retry rm still fails, and the ladder degrades to could not remove … (permission repair failed; sudo: ok) with the leftover surviving the job it was supposed to heal in.
Witness:
chmod-rung-deletion mutant: Tests 8 passed (8)
root:root mutant: Tests 8 passed (8)
+ toContain('sudo -n chown -R "$(id -u):$(id -g)" "$abs"'): 1 failed | 7 passed on the mutant, 8/8 on clean
Suggested fix — pin both sudo rung lines in full:
expect(reviewCleanCode).toContain('sudo -n chown -R "$(id -u):$(id -g)" "$abs" 2>/dev/null || true');
expect(reviewCleanCode).toContain('sudo -n chmod -R u+rwX "$abs" 2>/dev/null || true');or add a capability-gated fixture (probe sudo -n true) that makes a leftover foreign-owned with a 0555 inner dir and asserts removal.
中文说明
sudo 梯级只被动词子串 sudo -n chown -R 钉住；两种变异都能 8/8 全绿上线。（1）其后半段 sudo -n chmod -R u+rwX "$abs" 可被整行删除：没有任何断言提到 sudo -n chmod（chmod 正则/顺序断言只匹配非 sudo 梯级），且所有夹具都把父目录 chmod-555，即使在免密 sudo 主机上也没有夹具能观察到该梯级。（2）chown 属主目标 "$(id -u):$(id -g)" 可被变异为 root:root：没有任何断言捕获 id -u/id -g，也没有夹具断言 warning 的 owner: 字段。两种情况下，在有免密 sudo 的池成员上——正是该梯级要修复的成员——带模式锁定内目录的他人属主残留要么只被 chown 而不可写，要么被直接锁给 root，runner 用户的重试 rm 仍然失败，梯子降级为 could not remove … (permission repair failed; sudo: ok)，残留在本应修复它的 job 中存活下来。
证据：删除 chmod 梯级的变异 8/8 全绿；root:root 变异 8/8 全绿；加上 toContain('sudo -n chown -R "$(id -u):$(id -g)" "$abs"') 后，变异下 1 失败 | 7 通过，干净代码 8/8。
建议修复——完整钉住两条 sudo 梯级：
expect(reviewCleanCode).toContain('sudo -n chown -R "$(id -u):$(id -g)" "$abs" 2>/dev/null || true');
expect(reviewCleanCode).toContain('sudo -n chmod -R u+rwX "$abs" 2>/dev/null || true');或添加能力门控夹具（探测 sudo -n true）：把残留做成他人属主、内目录 0555，并断言其被删除。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // sudo line. | ||
| expect(reviewCleanCode).toMatch(/^\s*chmod -R u\+rwX "\$abs"/m); | ||
| expect(reviewCleanCode).toContain('sudo -n chown -R'); | ||
| expect(reviewCleanCode).toContain('remove_review_tree "$leftover"'); |
There was a problem hiding this comment.
[Suggestion] The leftover loop's glob — the call site that feeds every permission-poisoned tree into the ladder — is the only glob site in the sweep pinned to nothing. The sibling sites are tied to worktreePrefix (the bulk rm, the awk filter's index, the re-anchor case arm); the only pin on the loop is the call string. A typo or layout rename (for leftover in .qwen/tmp/review-pr-*.bak) matches nothing, so no poisoned tree ever reaches the ladder, and every pin and all eight fixtures stay green — the fixtures invoke the function directly and never execute the loop. Outcome: the foreign-owned EACCES leftover survives the sweep and kills the next review at checkout — the exact incident this diff cites as the reason the ladder exists (run 32577821716, the review of PR 9718).
Witness:
mutant glob '*.bak': Tests 8 passed (8)
+ toContain(`for leftover in ${worktreePrefix}*; do`): 1 failed | 7 passed on the mutant; mutant reverted + pin kept: 8/8
Suggested fix:
expect(reviewCleanStep).toContain(`for leftover in ${worktreePrefix}*; do`);中文说明
残留循环的 glob——把每棵权限投毒树喂给梯子的调用点——是清扫中唯一没有被任何断言钉住的 glob 位点。兄弟位点都绑定到 worktreePrefix（批量 rm、awk 过滤器的 index、重锚定 case 分支）；该循环上唯一的断言只有调用字符串。一个笔误或目录改名（for leftover in .qwen/tmp/review-pr-*.bak）匹配不到任何东西，于是没有任何投毒树进入梯子，所有断言和全部八个夹具依旧为绿——夹具直接调用函数，从不执行这个循环。后果：他人属主的 EACCES 残留在清扫后存活，并在下一次 review 的 checkout 时将其杀死——正是本 diff 引用的、梯子赖以存在的事故（run 32577821716，即 PR 9718 的 review）。
证据：变异 glob *.bak 下 8/8 全绿；加上 toContain(for leftover in ${worktreePrefix}*; do) 后，变异下 1 失败 | 7 通过；还原变异并保留断言后 8/8。
建议修复：
expect(reviewCleanStep).toContain(`for leftover in ${worktreePrefix}*; do`);— qwen3.8-max via Qwen Code /review (v0.22.0)
| const guardPos = reviewCleanCode.indexOf('if [ -n "$reason" ]'); | ||
| const chmodPos = reviewCleanCode.indexOf('chmod -R u+rwX "$abs"'); | ||
| const sudoPos = reviewCleanCode.indexOf('sudo -n chown -R'); |
There was a problem hiding this comment.
[Suggestion] No pin or fixture fixes the initial rm -rf "$abs" as PRECEDING the refusal guard. A guard-first rewrite (pre-guard rm moved below if [ -n "$reason" ]) passes bash -n, keeps the rm count 3, leaves all order pins intact, and ships 8/8 green with byte-identical fixture observables — silently dropping the diff's own documented behavior ("with a writable parent the first rung unlinks the link itself, which is correct"). The reachability is real: a registered worktree whose path is a symlink reaches the remove_review_tree fallback and trips the guard where a plain rm would have removed it. The whole-step A/B measured the real production delta: one spurious refusal warning plus silently-vs-refused removal — the trailing unguarded glob rm converges the end state, so no registration/branch leak (the leak chain claimed in review was refuted by probe). The documented behavior still regresses green.
Witness:
guard-first mutant: Tests 8 passed (8), byte-identical fixture observables
whole-step A/B: end state identical (link removed, registrations 0, branches 0); mutant adds one spurious refusal warning
position pin / writable-parent symlink fixture: red on the mutant, green on clean (9/9)
Suggested fix:
expect(reviewCleanCode.indexOf('rm -rf "$abs"')).toBeLessThan(guardPos);and/or a fourth fixture: a symlinked leftover over a WRITABLE parent, asserting no warning, the link unlinked, and the target intact.
中文说明
没有任何断言或夹具固定首个 rm -rf "$abs" 必须先于拒绝守卫。守卫前置的改写（把守卫前的 rm 移到 if [ -n "$reason" ] 之下）能通过 bash -n，rm 计数仍为 3，所有顺序断言保持成立，8/8 全绿上线且夹具可观测量逐字节相同——悄悄丢弃了本 diff 自己记录的行为（"父目录可写时，第一梯级会直接解除链接本身，这是正确的"）。可达路径真实存在：路径为符号链接的已注册 worktree 会走到 remove_review_tree 回退，并在普通 rm 本可删除它时触发守卫。对整个步骤做 A/B 测得真实生产差异：多出一条多余的拒绝 warning、静默删除变为拒绝删除——其后的无防护 glob rm 使终态收敛，因此没有注册/分支泄漏（评审中声称的泄漏链已被探针否定）。但被记录的行为仍会以全绿回归。
证据：守卫前置变异下 8/8 全绿、夹具可观测量逐字节相同；整步 A/B 终态一致（链接被删、注册 0、分支 0），变异仅多一条多余的拒绝 warning；位置断言/可写父目录符号链接夹具在变异下红、干净代码下绿（9/9）。
建议修复：
expect(reviewCleanCode.indexOf('rm -rf "$abs"')).toBeLessThan(guardPos);以及/或者第四个夹具：可写父目录上的符号链接残留，断言无 warning、链接被解除、目标完好。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(reviewCleanCode).toContain( | ||
| "command -v sudo >/dev/null 2>&1 || sudo_probe='absent'", | ||
| ); | ||
| expect(reviewCleanCode).toContain("sudo_probe='ok'"); |
There was a problem hiding this comment.
[Suggestion] The sudo-state pins capture all three sudo_probe assignments but not the sudo -n true predicate that separates ok from password-gated. A simplification to if command -v sudo >/dev/null 2>&1; then sudo_probe='ok' keeps all pins and fixtures green. On a runner that has sudo but no NOPASSWD entry — the exact incident class this PR exists for (run 32577821716) — the failure warning then reports sudo: ok instead of sudo: password-gated, sending the on-call to chase a chown failure instead of writing the sudoers rule — the mis-triage the comment directly above this pin says it exists to prevent.
Witness (probe-measured on this password-gated host):
predicate-drop mutant: Tests 8 passed (8)
original warning: sudo: password-gated — mutant warning: sudo: ok
suggested pin: green on the original, red on the mutant
Suggested fix:
expect(reviewCleanCode).toContain('command -v sudo >/dev/null 2>&1 && sudo -n true 2>/dev/null');中文说明
sudo 状态断言捕获了全部三个 sudo_probe 赋值，但没有捕获区分 ok 与 password-gated 的 sudo -n true 判定条件。简化为 if command -v sudo >/dev/null 2>&1; then sudo_probe='ok' 后，所有断言和夹具仍为绿。在有 sudo 但没有 NOPASSWD 条目的 runner 上——正是本 PR 要处理的事故类别（run 32577821716）——失败 warning 会报告 sudo: ok 而非 sudo: password-gated，让值班人员去追查 chown 失败，而不是去写 sudoers 规则——正是该断言上方注释声称要防止的误判。
证据（在本密码受控主机上实测）：删除判定条件的变异 8/8 全绿；原始 warning 为 sudo: password-gated，变异为 sudo: ok；建议断言在原始代码下绿、变异下红。
建议修复：
expect(reviewCleanCode).toContain('command -v sudo >/dev/null 2>&1 && sudo -n true 2>/dev/null');— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Relative input: the leftover loop's glob entries are relative. | ||
| const out = runRemoveReviewTree(fixture, '.qwen/tmp/review-pr-101'); |
There was a problem hiding this comment.
[Suggestion] No fixture invokes remove_review_tree with a path outside $GITHUB_WORKSPACE, and no pin covers the guard's *) rel='' arm — the "outside the workspace" refusal branch is pinned and executed by nothing. All three fixtures pass only in-workspace paths. A future rewrite breaking that arm (e.g. *) rel="${abs#/}" ;;, or dropping it) keeps every pin and fixture green; the uncovered surface is the arm plus its diagnostic reason — the defense-in-depth the guard exists for (the sudo leg escalates to root, and the guard is what keeps it inside the workspace).
Witness:
mutants *) rel="${abs#/}" and arm-dropped: shipped suite 8/8 green, both
proposed fixture: red on both mutants, green on clean (9/9)
Suggested fix: add an outside-workspace fixture — a second mkdtempSync sibling tree, chmod-555 parent so rm fails and the guard fires; assert status 0 and exactly one (outside the workspace) refusal warning (assert on the refusal reason, not "sibling intact": the first unprivileged rm runs before the guard and empties contents). Optionally pin *) rel='' textually.
中文说明
没有夹具用 $GITHUB_WORKSPACE 之外的路径调用 remove_review_tree，也没有断言覆盖守卫的 *) rel='' 分支——"工作区之外"的拒绝分支既没有被钉住，也没有被执行。三个夹具都只传工作区内路径。未来任何破坏该分支的改写（如 *) rel="${abs#/}" ;;，或直接删掉它）都能让所有断言和夹具保持为绿；未覆盖的面是该分支及其诊断原因——正是守卫存在的纵深防御（sudo 梯级会提权到 root，守卫负责把它限制在工作区内）。
证据：变异 *) rel="${abs#/}" 与整分支删除两种情况下，现有套件均 8/8 全绿；建议夹具在两种变异下红、干净代码下绿（9/9）。
建议修复：添加工作区外夹具——第二个 mkdtempSync 兄弟树，父目录 chmod-555 使 rm 失败从而触发守卫；断言 status 0 且恰有一条 (outside the workspace) 拒绝 warning（断言拒绝原因，而不是"兄弟目录完好"：首个非特权 rm 先于守卫执行，会清空内容）。可选：文本钉住 *) rel=''。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round, batched with the other three fixture findings. This round closed the Critical and the pin-level gaps (round batch bound); the outside-the-workspace refusal fixture is implemented next round. Until then the *) rel='' arm and its diagnostic reason stay pinned by nothing — no change this round alters that.
中文说明
推迟到下一轮，与其余三个夹具发现合并处理。本轮关闭了 Critical 与断言级缺口（单轮批处理上限）；工作区外拒绝夹具将在下一轮实现。在此之前 *) rel='' 分支及其诊断原因仍然没有任何断言覆盖——本轮没有任何改动改变这一点。
| const link = join(leftoverDir, 'review-pr-102'); | ||
| symlinkSync(target, link); |
There was a problem hiding this comment.
[Suggestion] No fixture exercises the function's existence-check -L arm ([ -e "$abs" ] || [ -L "$abs" ] || return 0): the one-edit mutant [ -e "$abs" ] || return 0 ships 8/8 green (every fixture path EXISTS, so -L never fires), and the loop's sibling-check mutant [ -e "$leftover" ] || continue ships green too. A dangling symlink named .qwen/tmp/review-pr-N over a writable parent: shipped code removes it (the pre-guard rm -rf unlinks the link); the mutant hits [ -e ] false and silently return 0 — the leftover survives with no warning, defeating the sweep's remove-or-named-warning contract with zero visibility. The existing symlink fixture (live target) does NOT discriminate: [ -e ] is true there, so the mutant passes it.
Witness:
function-check mutant: Tests 8 passed (8); loop-check mutant: Tests 8 passed (8)
clean + dangling fixture: Tests 10 passed (10); mutant + fixture: 1 failed | 9 passed (link left behind)
Suggested fix:
const dangling = join(leftoverDir, 'review-pr-104');
symlinkSync(join(fixture, 'nowhere'), dangling);
// assert status 0, stdout === '', and lstatSync(dangling, { throwIfNoEntry: false }) falsyOptionally pin both existence-check lines (the function's and the loop's) textually.
中文说明
没有夹具执行函数的存在性检查 -L 分支（[ -e "$abs" ] || [ -L "$abs" ] || return 0）：单行变异 [ -e "$abs" ] || return 0 可以 8/8 全绿上线（所有夹具路径都存在，-L 从不触发）；循环中兄弟检查的变异 [ -e "$leftover" ] || continue 同样全绿。可写父目录上的悬空符号链接 .qwen/tmp/review-pr-N：现有代码会删除它（守卫前的 rm -rf 解除链接）；变异则命中 [ -e ] 为假并静默 return 0——残留在无任何 warning 的情况下存活，以零可见性破坏清扫的"要么删除、要么点名 warning"契约。现有的符号链接夹具（目标存在）无法区分：那里 [ -e ] 为真，变异照样通过。
证据：函数检查变异 8/8 全绿；循环检查变异 8/8 全绿；干净代码 + 悬空链接夹具 10/10；变异 + 夹具 1 失败 | 9 通过（链接被留下）。
建议修复：
const dangling = join(leftoverDir, 'review-pr-104');
symlinkSync(join(fixture, 'nowhere'), dangling);
// 断言 status 0、stdout === ''，且 lstatSync(dangling, { throwIfNoEntry: false }) 为假可选：文本钉住两处存在性检查行（函数内与循环内）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round, batched with the other three fixture findings. This round closed the Critical and the pin-level gaps (round batch bound); the dangling-symlink fixture exercising the -L existence-check arm lands next round. The cited one-edit mutants remain green until then; nothing in this round's commit changes that.
中文说明
推迟到下一轮，与其余三个夹具发现合并处理。本轮关闭了 Critical 与断言级缺口（单轮批处理上限）；执行 -L 存在性检查分支的悬空符号链接夹具将在下一轮落地。在此之前所引用的单行变异仍然为绿；本轮提交没有任何改动改变这一点。
| // Make rm fail so the ladder reaches the refusal branch: with a | ||
| // writable parent the first rung unlinks the link itself, which is | ||
| // correct but never exercises the guard. | ||
| chmodSync(leftoverDir, 0o555); |
There was a problem hiding this comment.
[Suggestion] No test exercises the ladder's repair-success path: every failure fixture chmod-555s the PARENT directory, but the ladder only repairs $abs — so chmod -R u+rwX → retry → success is pinned textually only, never executed. A mutation that keeps every pinned substring yet breaks repair semantics ships 8/8 green — e.g. chmod -R u+rwX "$abs" --no-such-flag 2>/dev/null || true matches the pin regex (not end-anchored) and leaves the non-sudo repair rung dead: a mode-poisoned tree that chmod alone would heal degrades to the sudo rung or survives.
Witness:
chmod --no-such-flag mutant: Tests 8 passed (8)
proposed fixture (0o555 subdir inside the leftover): clean code → status 0, stdout '', tree gone; mutant → red
Suggested fix — a fourth behavioral test under the same gate: create leftover/sub at 0o555 holding a file; assert status === 0, stdout === '', existsSync(leftover) === false; restore modes in finally with an existence guard (on the success path the 0o555 subdir no longer exists).
中文说明
没有测试执行梯子的"修复成功"路径：所有失败夹具都把父目录 chmod-555，而梯子只修复 $abs——因此 chmod -R u+rwX → 重试 → 成功 只被文本钉住，从未被执行。任何保留全部被钉子串、却破坏修复语义的变异都能 8/8 全绿上线——例如 chmod -R u+rwX "$abs" --no-such-flag 2>/dev/null || true 能匹配该正则（未端锚定），使非 sudo 修复梯级失效：仅靠 chmod 就能修复的模式投毒树会降级到 sudo 梯级或直接残留。
证据：chmod --no-such-flag 变异下 8/8 全绿；建议夹具（残留内部放 0o555 子目录）：干净代码下 status 0、stdout 为空、树被删除；变异下红。
建议修复——在同一门控下增加第四个行为测试：创建含文件的 leftover/sub 并设为 0o555；断言 status === 0、stdout === ''、existsSync(leftover) === false；在 finally 中带存在性检查地恢复权限（成功路径下该 0o555 子目录已不存在）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Deferred to the next round, batched with the other three fixture findings. This round closed the Critical and the pin-level gaps (round batch bound); the repair-success fixture lands next round. Partial cover arrived this round anyway: the chmod-rung pin is now end-anchored with its guard, so the chmod --no-such-flag mutant cited here now fails the pinning test even before the fixture exists.
中文说明
推迟到下一轮，与其余三个夹具发现合并处理。本轮关闭了 Critical 与断言级缺口（单轮批处理上限）；修复成功夹具将在下一轮落地。本轮仍带来了部分覆盖：chmod 梯级断言现在带端锚定并包含其防护，因此这里引用的 chmod --no-such-flag 变异即便在夹具落地前也会使钉住测试失败。
…e ladder's guards (#9748)
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9748 (review round 4 feedback)One commit this round: DispositionsResolved in code
Deferred to the next round (answered on their threads)
Reason: this round already batches the Critical plus eight pin-level findings (the round bound). The four fixtures are one coherent batch — each constructs chmod/symlink state with cleanup in Observations
VerificationCommands actually run, with results:
中文说明轮次摘要 — PR #9748（第 4 轮评审反馈）本轮一个提交： 处置结果已在代码中解决
推迟到下一轮（已在各自线程答复）
原因：本轮已批处理 Critical 加八条断言级发现（达到单轮上限）。四个夹具构成一个内聚批次——每个都要构造 chmod/符号链接状态并在 观察
验证实际执行的命令及结果：
🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈（门自动测量）：bite 检查无法验证测试侧修复，该解决仅以轮次摘要为凭。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-6 resolves-through-symlinks refusal-arm fixture — same-line overlap drop at scripts/tests/review-worktree-cleanup-workflow.test.js:291 against comment 3837738183 (a different finding's thread); R4-6's own thread is comment 3837738180 (de…
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI (merge_group-only lanes) and the suite's CI run at this head died at checkout before collecting; local runs at HEAD were green (8/8 with realpath, 7 passed | 1 skipped under a realpath-stripped PATH model of the macOS lane).
Not explored to full depth (tool budget reached): "agent 1a": none — but note I could not run the vitest suite itself in this worktree (no node_modules ); the bash re-execution above substituted for the behavioral fixture….
Convergence: round 5 posted 5 inline comment(s), 2 of them reported for the first time; the previous round posted 13 (13 new). Findings keep coming back to the same files: scripts/tests/review-worktree-cleanup-workflow.test.js (findings in round 4; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. 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.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI (merge_group-only lanes) and the suite's CI run at this head died at checkout before collecting; local runs at HEAD were green (8/8 with realpath, 7 passed | 1 skipped under a realpath-stripped PATH model of the macOS lane)。
未探索到全部深度（达到工具调用预算）："agent 1a"：none — but note I could not run the vitest suite itself in this worktree (no node_modules ); the bash re-execution above substituted for the behavioral fixture…。
收敛情况：第 5 轮发布了 5 条行内评论，其中 2 条是首次提出；上一轮发布了 13 条（其中 13 条首次提出）。发现反复回到同一批文件：scripts/tests/review-worktree-cleanup-workflow.test.js（第 4 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const realpathAvailable = | ||
| spawnSync('realpath', ['--', '/'], { stdio: 'ignore' }).status === 0; |
There was a problem hiding this comment.
[Suggestion] No completed execution of the changed suite exists at this head. The file sits outside every npm workspace, so the workspace test commands the efficacy probe drives never collect it, and the one CI suite that does collect it (npm run test:scripts via test:ci) failed at checkout on the Test (ubuntu-latest, Node 22.x) check at this commit — an EACCES while removing a leftover probe workspace, environmental rather than caused by this diff. The net effect: if the PR merges while that check is red or skipped for environmental reasons, a defect in the new test code ships unobserved — e.g. if the remove_review_tree extraction anchors move (indexOf returning −1 makes the sliced function garbage passed to bash -c), nothing in the pipeline has executed these assertions. Obtain one real green execution before merge: clear the leftover .qwen/tmp/review-pr-9748-scratch-verify--round-1--* probe workspace from the self-hosted runner and re-run the Test job, or run npm ci && npm run test:scripts once on a Linux, non-root host.
中文说明
在当前 head 上，被改动的测试套件没有任何一次完整执行。该文件位于所有 npm workspace 之外，因此效能探测所驱动的 workspace 测试命令不会收集它；而唯一收集它的 CI 套件（test:ci 链式调用 npm run test:scripts）在本提交的 Test (ubuntu-latest, Node 22.x) 检查中于 checkout 阶段失败——删除残留探测工作区时发生 EACCES，属于环境问题，并非本 diff 所致。净效果：如果该检查因环境原因红着或被跳过时 PR 合入，新测试代码中的缺陷将在无人观察的情况下上线——例如 remove_review_tree 提取锚点移动时（indexOf 返回 −1 会使切出的函数变成传给 bash -c 的垃圾），流水线中没有任何环节执行过这些断言。合入前请获得一次真实的绿色执行：清掉自托管 runner 上残留的 .qwen/tmp/review-pr-9748-scratch-verify--round-1--* 探测工作区并重跑 Test 任务，或在 Linux 非 root 主机上运行一次 npm ci && npm run test:scripts。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| mkdirSync(leftover, { recursive: true }); | ||
| writeFileSync(join(leftover, 'leftover.txt'), 'x'); | ||
| // Relative input: the leftover loop's glob entries are relative. | ||
| const out = runRemoveReviewTree(fixture, '.qwen/tmp/review-pr-101'); |
There was a problem hiding this comment.
[Suggestion] R4-11: Still standing — no fixture invokes remove_review_tree with a path outside $GITHUB_WORKSPACE, and the guard's *) rel='' arm is pinned and executed by nothing: a rewrite breaking or dropping that arm keeps the suite 8/8 green (both mutants probed in round 4), removing the defense-in-depth that keeps the root-escalating sudo leg inside the workspace. The fixture was deferred per the thread reply and has not landed in this commit; the full evidence and the proposed fixture are in the original thread.
中文说明
R4-11：仍然存在——没有夹具用 $GITHUB_WORKSPACE 之外的路径调用 remove_review_tree，守卫的 *) rel='' 分支既无断言也无执行：破坏或删除该分支的改写都能让套件 8/8 全绿（第 4 轮已对两种变异做过探针），等于拆掉了把提权到 root 的 sudo 梯级限制在工作区内的纵深防御。夹具已按线程回复推迟，本提交未落地；完整证据与建议夹具见原线程。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const leftoverDir = join(fixture, '.qwen/tmp'); | ||
| mkdirSync(leftoverDir, { recursive: true }); | ||
| const link = join(leftoverDir, 'review-pr-102'); | ||
| symlinkSync(target, link); |
There was a problem hiding this comment.
[Suggestion] R4-12: Still standing — no fixture exercises the existence-check -L arm ([ -e "$abs" ] || [ -L "$abs" ] || return 0): the one-edit mutant [ -e "$abs" ] || return 0 ships with the suite 8/8 green, under which a dangling symlink review-pr-N over a writable parent survives silently with no warning, defeating the sweep's remove-or-named-warning contract. The dangling-symlink fixture was deferred per the thread reply and has not landed in this commit; the full evidence and the proposed fixture are in the original thread.
中文说明
R4-12：仍然存在——没有夹具执行存在性检查 -L 分支（[ -e "$abs" ] || [ -L "$abs" ] || return 0）：单行变异 [ -e "$abs" ] || return 0 能让套件 8/8 全绿，此时可写父目录下的悬空符号链接 review-pr-N 会静默存活（无 warning），破坏清扫的“要么删除、要么点名 warning”契约。悬空符号链接夹具已按线程回复推迟，本提交未落地；完整证据与建议夹具见原线程。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Make rm fail so the ladder reaches the refusal branch: with a | ||
| // writable parent the first rung unlinks the link itself, which is | ||
| // correct but never exercises the guard. |
There was a problem hiding this comment.
[Suggestion] No fixture observes the first rm -rf "$abs" actually executing. A rewrite that deadens that line while keeping its text — e.g. wrapping it in if false; then … fi — survives every pin and fixture: the count pin (3× rm -rf "$abs"), the order pin, and every substring pin still match; fixture 1's plain leftover is still removed by the chmod-rung retry, and fixtures 2/3 already expect the first rm to fail. Measured: with that mutant in the workflow the whole suite stays green (8 passed), while in production a symlinked leftover under a writable parent would be refused with a warning and left behind instead of unlinked, and every healthy leftover would incur a chmod -R permission mutation before removal. The comment above documents exactly this hazard — the pins chosen enforce textual order, not execution. One fixture where the symlinked leftover's parent stays writable (no chmod 555) — asserting out.status === 0, empty stdout, link gone, target intact — kills the mutant: verified it fails against the mutant with a refusal warning and passes on the current code (9 passed).
中文说明
没有任何夹具观察第一个 rm -rf "$abs" 的实际执行。一个让该行失效但保留其文本的改写——例如用 if false; then … fi 包裹——能通过所有断言与夹具：计数断言（3× rm -rf "$abs"）、顺序断言和所有子串断言依然匹配；夹具 1 的普通残留仍会被 chmod 梯级的重试删除，夹具 2/3 本来就期望第一个 rm 失败。实测：在 workflow 中植入该变异后整套测试仍全绿（8 passed），而生产环境中，可写父目录下的符号链接残留将被拒绝并遗留下来（而不是被解除链接），每个正常残留在删除前都会先经历一次 chmod -R 权限变更。上方注释恰好描述了这个隐患——所选断言钉住的是文本顺序，而非执行。补一个夹具：符号链接残留的父目录保持可写（不 chmod 555），断言 out.status === 0、stdout 为空、链接消失、目标完好——即可杀死该变异（已验证：对变异体失败并输出拒绝 warning，对当前代码通过，9 passed）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Make rm fail so the ladder reaches the refusal branch: with a | ||
| // writable parent the first rung unlinks the link itself, which is | ||
| // correct but never exercises the guard. | ||
| chmodSync(leftoverDir, 0o555); |
There was a problem hiding this comment.
[Suggestion] R4-13: Still standing — no test exercises the ladder's repair-success path: every failure fixture chmod-555s the PARENT directory, but the ladder only repairs $abs, so chmod -R u+rwX → retry → success is pinned textually only, never executed. The round-4 probe measured chmod -R u+rwX "$abs" --no-such-flag shipping with the suite 8/8 green while leaving the rung dead. The repair-success fixture was deferred per the thread reply and has not landed in this commit — and this round's reverse audit independently re-derived the same gap. The full evidence and the proposed fixture are in the original thread.
中文说明
R4-13：仍然存在——没有测试执行梯子的“修复成功”路径：所有失败夹具都把父目录 chmod-555，而梯子只修复 $abs，因此 chmod -R u+rwX → 重试 → 成功 只有文本钉住，从未被执行。第 4 轮探针显示 chmod -R u+rwX "$abs" --no-such-flag 能让套件 8/8 全绿同时使该梯级失效。修复成功夹具已按线程回复推迟，本提交未落地——本轮反向审计也独立重新推导出了同一缺口。完整证据与建议夹具见原线程。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- resolves-through-symlinks refusal-arm fixture gap — already reported (comment 3837738180, thread at scripts/tests/review-worktree-cleanup-workflow.test.js:291)
- unguarded warning echoes under the leftover loop's errexit — already reported (comment 3836891764)
- unhealable tree re-laddered identically by the leftover loop (doubled repair cost + duplicate warning) — already recorded in round 3's deferral list (review 5001347650)
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
.github/workflows/qwen-code-pr-review.yml:1925 — [probe] D6-1 failure warning mis-triages when the parent directory is the blocker — rungs repair only $abs, but unlink needs write on the parentscripts/tests/review-worktree-cleanup-workflow.test.js:285 — [probe] D6-2 no pin requires remove_review_tree to be defined before its call sites; a reorder survives every test and dies as command-not-found in the failure pathscripts/tests/review-worktree-cleanup-workflow.test.js:405 — [review] R4-11 still standing — no fixture invokes remove_review_tree with a path outside $GITHUB_WORKSPACE; the guard's *) rel='' arm is pinned and executed by nothingscripts/tests/review-worktree-cleanup-workflow.test.js:426 — [review] R4-12 still standing — no fixture exercises the existence-check -L arm; a dangling symlink over a writable parent survives silentlyscripts/tests/review-worktree-cleanup-workflow.test.js:429 — [review] R5-2 still standing — no fixture observes the first rm -rf actually executing; an if-false wrapper survives every pin and fixturescripts/tests/review-worktree-cleanup-workflow.test.js:430 — [review] R4-13 still standing — no test exercises the ladder's repair-success path; every failure fixture chmod-555s the parent, which the rungs never touch
中文说明
已审查——无阻断问题。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…ixtures (#9748) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #9748 (round: 5 inline findings)All five inline findings were triaged as act. They share the root cause the round-5 convergence note names: the repair ladder's arms were pinned only textually and never executed. This round adds the four behavioral fixtures that execute them, plus the green execution the first finding asked for. No workflow change was needed — every fix is a test-side witness. Commit: Findings and dispositions
Mutation probes (temporarily mutated
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
scripts/tests/review-worktree-cleanup-workflow.test.js:150 — [probe] removeReviewTreeFn extraction truncates silently at the first bare-} line; fixtures then die with a misleading bash EOF errorscripts/tests/review-worktree-cleanup-workflow.test.js:252 — [probe] leftover loop's existence guard unpinned and unexecuted; dropping the -L arm ships green and silently skips dangling-symlink survivors
[Critical] R7-1 The two skip-warning echoes in the registered-worktree loop of the 'Clean review worktrees' step — echo "::warning::skipping suspicious review worktree path: $worktree" (.github/workflows/qwen-code-pr-review.yml:1942) and echo "::warning::skipping unexpected review worktree path: $worktree" (line 1947) — emit the untrusted registered path raw. The CR/LF stripping this PR added covers only the two ${abs//[$'\r\n']/ } expansions inside remove_review_tree; these sibling echoes in the same step have none. read -r blocks LF but not CR, and the Actions runner splits step stdout on bare CR, so a CR-bearing registered worktree path injects a standalone workflow-command line into this privileged job's stdout. Witness (real step body at HEAD, fixture with 3 hostile registrations): standalone injected command lines "::stop-commands::pwned", "::endgroup::", "::notice::forged/git" — 3 of 3; the registrations survive the sweep and re-inject on every later run. Suggested fix: the PR's own idiom on both echoes — ${worktree//[$'\r\n']/ } — plus extending the contract pin's direct-interpolation scan to these two lines. (Anchoring note: the echo lines sit outside every diff hunk, so this Critical rides in the body.)
中文说明
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
[Critical] R7-1 The two skip-warning echoes in the registered-worktree loop of the 'Clean review worktrees' step — echo "::warning::skipping suspicious review worktree path: $worktree" (.github/workflows/qwen-code-pr-review.yml:1942) and echo "::warning::skipping unexpected review worktree path: $worktree" (line 1947) — emit the untrusted registered path raw. The CR/LF stripping this PR added covers only the two ${abs//[$'\r\n']/ } expansions inside remove_review_tree; these sibling echoes in the same step have none. read -r blocks LF but not CR, and the Actions runner splits step stdout on bare CR, so a CR-bearing registered worktree path injects a standalone workflow-command line into this privileged job's stdout. Witness (real step body at HEAD, fixture with 3 hostile registrations): standalone injected command lines "::stop-commands::pwned", "::endgroup::", "::notice::forged/git" — 3 of 3; the registrations survive the sweep and re-inject on every later run. Suggested fix: the PR's own idiom on both echoes — ${worktree//[$'\r\n']/ } — plus extending the contract pin's direct-interpolation scan to these two lines. (Anchoring note: the echo lines sit outside every diff hunk, so this Critical rides in the body.)
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 282 passed · 0 failed · 282 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：282 通过 · 0 失败 · 282 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9748 Deep Verification —
|
| # | Scenario (oracle) | base (HEAD^1) |
head (HEAD^2) |
|---|---|---|---|
| A | Leftover locked 0555 — incident shape (tree exists? warning names it? exit) |
survives, silently — exit 0, no warning | repaired + removed, exit 0, zero warnings |
| B | Healthy leftover (parity) | removed | removed |
| C | Unrepairable: parent dir locked (warning text? exit?) | survives, silently | survives + ::warning::could not remove … (sudo: absent; owner: node), exit 0 |
| G/H | Registered poisoned worktree (tree? registration? branch?) | warned could not remove, tree survives |
repaired + removed; registration pruned; qwen-review/* branch deleted |
| D | Two poisoned siblings (both removed?) | n/a (silent) | both removed — one cannot mask the other |
The flip is clean: every permission-poisoned shape that base silently inherits, head either repairs+removes (A, G) or names in a warning with the deciding state (C) — always exit 0.
Reviewer Test Plan, per step: step 1 (contract suite) — executed, 12/12, see below. Step 2 (extracted script vs synthetic workspaces) — reproduced cell-by-cell above, including the incident-shape tree, the healthy leftover, and the symlink refusal with target untouched; exit 0 in all 13 scenarios. Step 3 (pool run with passwordless sudo) — not executable here (no root, no sudo binary in-container); the wiring was shim-verified instead (below), and the author themselves marks this step as pending a real pool run.
Safety legs of the ladder
| Probe | Result |
|---|---|
Symlinked leftover, locked parent → refuse (path is a symlink), target file intact, exit 0 |
✅ E1–E4 |
Workspace itself under a symlinked ancestor (macOS /tmp case) → not refused, tree repaired |
✅ F1–F3 |
Symlinked intermediate dir below workspace → refuse (resolves through symlinks); escalating legs never run |
✅ L1–L4 |
Absolute path outside the workspace (direct fn call) → refuse (outside the workspace), foreign tree untouched |
✅ K1–K3 |
Hostile name review-pr-\n::error::injected, locked parent → exactly one warning line, zero bare ::error:: lines, exit 0 |
✅ I1–I4 |
CR sibling (review-pr-\r::error::…, runner splits on bare CR too) → single segment, zero ::error::, exit 0 |
✅ I5–I7 |
| ESC-byte name → stays on the single warning line, exit 0 (no CR/LF ⇒ no new command line) | ✅ M1–M2 |
sudo probe states via PATH shims: absent (natural here), password-gated (probe called once, no chown attempted), ok (chown -R 1000:1000 then chmod -R u+rwX, exact args, order pinned from logs/shim-ok.log) |
✅ J1–J10 |
| No escalation: chmod-repairable tree removed with zero sudo invocations (shim log empty) | ✅ J11–J13 |
The sudo rung's real root-level effect (repairing a genuinely foreign-owned tree) is untestable in this container — see Not covered.
Contract suite + mutation matrix (vacuity)
Gate: npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/review-worktree-cleanup-workflow.test.js → 12/12 passed, 0 skipped (permission fixtures actually executed: uid 1000, Linux, realpath present). Witness evidence/03-contract-suite-12-of-12.png, log logs/contract-suite-head.txt.
Mutation A/B (harness/run-mutations.mjs, one-point mutants of the production YAML, suite re-run per mutant, YAML restored byte-identical — git status clean at the end): 9/9 killed, 0 survivors, control green. Witness evidence/02-mutation-matrix-9-of-9-killed.png, per-mutant logs logs/mutant-*.txt.
| Mutant | Result | First red test |
|---|---|---|
| M0 control (none) | green 12/12 | — |
| M1 drop retry-rm after chmod rung | KILLED 1F | pinning test (rm count / adjacency pins) |
| M2 drop chmod rung | KILLED 2F | pinning test + behavioral: repair fixture got ::warning::could not remove… where it expected '' — intended mismatch |
M3 invert refusal comparison !=→= |
KILLED 3F | pinning test |
| M4 drop CR/LF sanitization (both warnings) | KILLED 2F | pinning test + behavioral: injection fixture emitted 2 lines where 1 expected — intended mismatch |
M5 drop sudo -n true predicate |
KILLED 1F | pinning test |
M6 drop unconditional final return 0 |
KILLED 1F | pinning test |
M7 blank rel derivation |
KILLED 4F | pinning test |
| M8 rename leftover glob (positive control, pin-level) | KILLED 1F | pinning test |
M9 drop -L dangling-link existence arm (positive control, behavioral) |
KILLED 1F | removes a dangling symlink via the -L existence arm |
Both positive controls landed in the seam they police (M9 red in the behavioral fixture itself), and the two behavioral kills quote expected-versus-actual values, so the red runs fail the intended assertions — not imports or setup. No layered-guard combination row was needed: every guard is individually load-bearing (each single-hunk revert changed an outcome), so no two hunks defend one hazard redundantly.
Targeted gates
| Gate | Result |
|---|---|
| Contract suite (above) | 12/12 |
workflow-size.test.js (consumes .size-baseline) |
181/181 |
Size value: wc -c qwen-code-pr-review.yml = 158010 == baseline entry 158010 |
exact match |
bash --noprofile --norc -n on both extracted steps |
clean; live-proven — planted dropped closing quote → exit 2 syntax error |
| actionlint 1.7.12 on the changed workflow | clean; live-proven (planted bad if: expr → exit 1) |
shellcheck 0.11.0, -S warning, both extracted steps |
clean; live-proven (planted SC2154). One info-level SC2012 on head — see Findings |
node scripts/lint.js --shellcheck (repo wrapper) |
exit 0 (only pre-existing warnings in scripts/test-rewind-e2e.sh, untouched by this PR) |
| eslint on the changed test file | clean; live-proven (planted parse error → exit 1) |
Findings (by severity — none blocking)
- (info, pre-existing, not introduced by this PR) The pre-guard best-effort
rm -rf— both the firstrminsideremove_review_treeand the step's bulkrm -rf .qwen/tmp/review-pr-*line, which exists identically on base — follows a symlinked intermediate directory planted below the workspace (e.g..qwen→ elsewhere) and unlinks content at the target before the refusal guard fires. Measured: contents removed, tree left, refusal(resolves through symlinks)printed afterwards. This is not a regression (base behaves byte-identically — an A/A observation), involves no privilege escalation (plain userrm), and the guard's stated job — keeping the escalatingchmod/sudo chown/chmod -Rlegs off redirected paths — is verified intact (cells L2–L4). The rm-before-guard ordering is itself pinned by the contract test ("a guard-first rewrite refuses a symlinked leftover that the plain rm would simply have unlinked"). Recorded so a reviewer who reads "refused rather than repaired" as "never touched" has the exact boundary: refusal gates repair, the pre-existing best-effort unlink still runs first. - (info) The new failure warning's owner extraction trips shellcheck SC2012 (use find instead of ls, info severity —
-S warningis clean) atowner: $(ls -ld "$abs" … | awk 'NR==1 {print $3}'). Cosmetic here:ls -ldis used positionally (field 3 is the owner, always before the filename) and theNR==1pin keeps newline-bearing names on one line. No action required.
Not covered
- Real passwordless-sudo repair of a genuinely foreign-owned tree — needs root; this container is uid 1000 with no sudo binary. The rung was shim-verified for wiring, argument shape, ordering, probe states, and no-escalation; the kernel-level effect awaits a pool member with NOPASSWD (the author's own step 3).
- yamllint —
node scripts/lint.js --setupcould not install it in-container (pip3: Permission denied); the wrapper check exits 1 for that environmental reason. Not a PR signal. - Repo-wide gates (full monorepo test suite, prettier, typecheck) — left to the PR's own CI; this round ran the gates for the changed surface only. The no-arg
node scripts/lint.jsform was deliberately not run (it rewrites the tree via prettier). - Per-commit attribution — depth-2 clone: 1 of the 8 commits in the metadata snapshot is locally reachable. The aggregate
HEAD^1..HEADdiff is what was verified. No live-main trial merge either (no network); the base is main's tip as of the merge ref. - Calibration against a real production artifact — the sweep emits no stored artifact and the incident run's log is not retrievable without a token; the replay reproduces the incident's shape (poisoned tree surviving base's sweep silently), not its cause (a container job leaving a foreign-owned tree — needs root).
Methodology
Environment: the verify container itself (node:22-bookworm, node v22.23.2, uid 1000 node, no sudo binary, realpath/bash/git present) — a live sample of the lane runtime. The base and head step scripts were extracted from jobs['review-pr'] → 'Clean review worktrees' via the yaml package (never retyped), base from git show HEAD^1:…. Every sweep cell ran the extracted script in a fresh mktemp workspace (git init + one commit) under bash --noprofile --norc -e with GITHUB_WORKSPACE = cwd, asserting exit code, stdout lines, and post-state on disk. The sudo leg was exercised through PATH shims that log every invocation and (for -n chmod) enact it; the shim logs (logs/shim-*.log) are quoted above. Mutants rewrote the production YAML in place and restored it byte-identically after each iteration. Harnesses live in harness/ (ab-harness.sh, run-mutations.mjs, extract-step.mjs), raw per-cell and per-mutant logs in logs/, image evidence in evidence/.
Flakiness gate log
rounds=5 files=1 skipped=0
file scripts/tests/review-worktree-cleanup-workflow.test.js: (cd .) npx --no-install vitest run --config ./scripts/tests/vitest.config.ts ./scripts/tests/review-worktree-cleanup-workflow.test.js
per-file results (P=pass F=fail I=infra-exit, one letter per run):
scripts/tests/review-worktree-cleanup-workflow.test.js: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
round 2 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
round 3 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
round 4 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
round 5 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9748 (round: address-review)Commit: Feedback dispositions[rv:5002252799] CHANGES_REQUESTED — R7-1 [Critical]: the two skip-warning echoes in the registered-worktree loop emit the untrusted registered path raw → Resolved in code.
[rv:5002252799] Two deferred probe notes (removeReviewTreeFn extraction truncation; leftover-loop No inline comments, issue-level comments, failed checks, or still-red checks were present this round. Critical-only mode noted; the Critical above was the only actionable item. Adjacent discovery flagged for follow-up (not changed — out of this PR's footprint)
Files changed
VerificationCommands actually run and their results:
中文说明Autofix 审查轮次 — PR #9748（轮次：address-review）提交： 反馈处理[rv:5002252799] CHANGES_REQUESTED — R7-1 [Critical]：registered-worktree 循环中的两条 skip 警告 echo 原样输出了不可信的注册路径 → 已在代码中修复。
[rv:5002252799] 两条延后的探针记录（removeReviewTreeFn 提取截断；leftover 循环 本轮没有行内评论、issue 级评论、失败检查或持续红检查。已知悉 Critical-only 模式；上述 Critical 是唯一可执行条目。 顺带发现、建议后续跟进（未改动——超出本 PR 足迹）
变更文件
验证实际执行的命令及结果：
Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review Complete — PR #9748
Verdict: Comment — no blocking issues found. 5 suggestion-level findings.
Reviewed commit: 8fd03427aeed6bcf9f927eae013595f36cfedbba
Model: deepseek-v4-flash (round 1 only; round 2 skipped because findings were identified)
What the PR does
Adds a permission-repair ladder (remove_review_tree()) to the review workflow's end-of-job worktree cleanup step. When rm -rf fails on a foreign-owned leftover tree, the ladder tries chmod -R u+rwX, then sudo -n chown -R + chmod -R on passwordless-sudo runners, each followed by a retry rm -rf. Symlinks and outside-workspace paths are refused before escalation. The step never fails the job.
Findings (all suggestion-level)
| # | File | Finding |
|---|---|---|
| 1 | scripts/tests/review-worktree-cleanup-workflow.test.js |
Missing pin for remove_review_tree "$worktree" in the registered-worktree loop (the "$leftover" analogue is pinned). |
| 2 | .github/workflows/qwen-code-pr-review.yml |
The resolves through symlinks guard is pinned by a substring assertion only and is not exercised by a fixture. |
| 3 | .github/workflows/qwen-code-pr-review.yml |
The path could not be resolved guard is pinned by nothing and is never exercised. |
| 4 | .github/workflows/qwen-code-pr-review.yml |
The final echo "::warning::..." in remove_review_tree is not guarded by ` |
| 5 | scripts/tests/review-worktree-cleanup-workflow.test.js |
Test file sits outside every npm workspace; confirm it is collected by the standard test runner in CI. |
Notes
- The automated inline-comment submission failed due to a network/API reachability issue, so findings are posted as a body-only review.
- No Critical or blocking issues were identified.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
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.
中文说明
未发现问题。LGTM！✅
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.0)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Reviewed the repair ladder and its guards at head 8fd0342:
- The ladder's order and refusals are right: plain rm first; then refuse-before-repair on any path that is outside the workspace, is itself a symlink, fails to resolve, or resolves through a symlink planted below the workspace — comparing against the workspace's OWN resolved path so legitimate symlinked ancestors (macOS /tmp → /private/tmp class) don't read as redirects; then chmod-and-retry, then passwordless-sudo chown/chmod-and-retry, then a named warning that never fails the job. Since the sudo leg escalates to root, refusing the ambiguous shapes instead of repairing them is the correct direction.
- The two round-2 Criticals are closed at this head: the warning-line sanitization now strips BOTH CR and LF from every
${abs}/${worktree}expansion (the runner splits stdout on bare CR too), and thels -ldenrichment is constrained toNR==1 {print $3}— the first line ofls -ldoutput carries only stat metadata before the name, so a newline-bearing leftover name cannot contribute a forged line or shift field 3; the realpath fixture gate (490bafe) keeps the behavioral tests off the merge_group macOS lane that ships no realpath(1). - Registered-path handling is sound:
..traversal rejected by pattern, re-anchored to the review prefix under the workspace, skip warnings stripped; glob survivors run through the ladder individually so one poisoned tree can't mask siblings; the final branch-warning interpolation is safe because git refnames cannot carry control characters.
One defense-in-depth nit, non-blocking: the $(ls -ld "$abs" | awk 'NR==1 {print $3}') substitution result itself isn't passed through the CR/LF strip — the only remaining way to get a newline through it is a username containing CR in the CLEANUP environment's own passwd database, which an attacker can't plant (the leftover's name rides in after field 8 and can't forge the owner field), so it's a belt-and-suspenders item rather than an open entrance.
CI at approval time: 15 checks passing, none failing; the automated review hasn't re-run on this head yet, so the two stale Critical threads still show unresolved — both are addressed by the commits above.
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Autofix round 7 — review feedback addressedCommit: Feedback source: body-only review Findings and dispositions1 — Missing pin for 2 — 3 — 4 — Final warning echo not guarded by 5 — Test file outside every npm workspace; confirm CI collection → DECLINED WITH EVIDENCE. Changes
Mutation probes (each new guard/branch witnessed)
Verification
Note on two failures in the first full-suite run: 中文说明Autofix 第 7 轮 — 评审反馈处理提交： 反馈来源：仅正文的评审 发现与处置1 — 注册工作树循环中缺少对 2 — 3 — 4 — 最后的警告 echo 未加 5 — 测试文件位于所有 npm workspace 之外；确认 CI 收集 → 以证据拒绝。 变更内容
变异探针（每个新守卫/分支均有见证）
验证
关于首次全套件运行中 2 个失败的说明： Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Released in v0.22.2. |
5 participants
What this PR does
The review workflow's end-of-job worktree sweep now repairs permissions before giving up on a removal. When deleting a leftover review worktree fails with a permission error, the cleanup tries to make the tree removable — restoring write bits on what the job's own user owns, then escalating to passwordless sudo for ownership repair where the pool member allows it — and retries the removal. Paths that resolve through symlinks are refused instead of repaired, because the sudo leg escalates to root. When nothing works, the sweep names the survivor in a warning rather than leaving it silently, and it never fails the job.
Why it's needed
Shared review runners reuse one workspace, and a containerised job on the pool can leave a review worktree owned by another uid and read-only. The sweep used to give up on the first EACCES, so the poison survived into the next job, whose checkout then died on it. Measured live: the review of PR #9718 failed after seventeen seconds because checkout could not wipe a leftover verifier scratch tree holding files the job's user could not unlink, and both existing self-heal legs in the workflow degraded because that pool member has no passwordless sudo. Pre-checkout ownership restore already tries to fix this class; this hands the same responsibility to the cleanup that is supposed to remove the trees in the first place, so the mess is cleared by the job that owns the removal instead of being inherited by the next one.
Reviewer Test Plan
How to verify
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/review-worktree-cleanup-workflow.test.js).0555) was repaired and removed; a healthy leftover was swept; a.qwen/tmpredirected through a symlink was refused with a warning and its link target left untouched; the step exited 0 in every scenario.Evidence (Before & After)
N/A (non-user-visible CI change; see the functional-harness results in "How to verify").
Tested on
Environment (optional)
Workflow change only: vitest contract tests plus a bash functional harness against the extracted step script. Linux is marked untested because the real pool run is still pending; the script uses only coreutils available on the pool (
realpath,chmod,chown,sudo -n).Risk & Scope
Linked Issues
Refs the failed run that motivated this: run 32577821716 (review of PR #9718).
中文说明
这个 PR 做了什么
Review workflow 在 job 结尾清扫 worktree 时，现在会先修复权限再放弃删除。当删除残留的 review worktree 因权限错误失败时，清理会先尝试让该目录变得可删——恢复 job 自身用户所拥有的条目的写权限，在池内成员允许时再通过免密 sudo 修复属主——然后重试删除。对通过符号链接解析的路径则拒绝修复，因为 sudo 分支会提权到 root。如果所有手段都失败，清扫会用 warning 点名残留目录，而不是静默放过，且永远不会让 job 失败。
为什么需要
共享的 review runner 复用同一个工作区，池里的容器化 job 可能把 review worktree 留成其他属主且只读。过去清扫遇到第一个 EACCES 就放弃，毒物因此留给下一个 job，后者的 checkout 会被它杀死。实测案例：PR #9718 的 review 在 17 秒后失败，因为 checkout 无法清掉一个残留的 verifier scratch 树——其中含有 job 用户无法 unlink 的文件，而 workflow 里已有的两条自愈腿都因该池成员没有免密 sudo 而降级。checkout 前的属主恢复步骤已经在尝试修这类问题；本次把同样的职责交给本应负责删除这些树的清理步骤，让残留由拥有删除职责的 job 自己清掉，而不是留给下一个 job 继承。
评审测试计划
如何验证
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/review-worktree-cleanup-workflow.test.js）。0555不可删）被修复并删除；正常残留被清扫；.qwen/tmp被符号链接重定向时拒绝修复并发出 warning，链接目标分毫未动；所有场景步骤均以 0 退出。前后对比证据
N/A（非用户可见的 CI 改动；功能验证结果见上文"如何验证"）。
测试环境
环境（可选）
仅 workflow 改动：vitest 契约测试 + 针对提取出的步骤脚本的 bash 功能验证。Linux 标记为未测，因为真实池上运行尚未发生；脚本只使用池上可用的 coreutils（
realpath、chmod、chown、sudo -n）。风险与范围
关联 Issue
动机来自这次失败运行：run 32577821716（PR #9718 的 review）。