refactor(ci): simplify the review checkout self-heal back to wipe-and-retry - #9327
Conversation
…-retry QwenLM#9220 fixed a real incident (a corrupt persisted workspace made seven review jobs fail checkout on the same missing SHAs), but eight review rounds grew the heal step from ~15 lines into ~60 lines of path-guard layers (realpath canonicalization, two trailing-slash strip loops, a denylist case, a RUNNER_WORKSPACE allowlist) plus ~450 lines of tests pinning their mutation resistance. Every removed layer defended against a mangled GITHUB_WORKSPACE. That variable is set by actions/runner; anything that could mangle it — a compromised runner, a step writing GITHUB_ENV — already executes arbitrary code on the machine and needs no wipe to do damage, so the guard cannot defend against the only actor able to trigger it. The realistic contract is the :? guard: fail loud on a dropped variable. Kept and still pinned by tests: the pool wipe idiom, the sudo fallback leg (exact argv), the never-fail exit contract with named survivors, the identical retry checkout, and the continue-on-error invariants. Also dropped with the guards: the GNU-only realpath flag and its host-probe test machinery.
|
Thanks for the PR — a tightly scoped cleanup. Template: all sections present ✓ Problem: real, though it's a complexity removal rather than a bug report. The guard layers removed here carry observable costs: a GNU-only Direction: aligned — simplicity-first, and this shrinks the heal step from ~60 lines to ~22. One flag up front: this deliberately reverts feedback accepted in the #9220 review rounds (R2–R8), so the owner of that decision should confirm the reversal — that lands in a maintainer's lap at the end of this review regardless (see the guardrail note in my final comment). Size: no core package paths touched — 51 production lines ( Approach: the scope is right — every edit either removes a guard layer or a test that pins one; no drive-by changes. What survives (the Risk: no high-risk path signals. Moving on to code review. 🔍 中文说明感谢贡献——这是一个范围收得很干净的清理 PR。 模板:各节齐全 ✓ 问题:真实存在,虽然这是复杂度移除而不是 bug 修复。被删除的守卫层有可观测的成本:GNU 专属的 方向:对齐——simplicity-first,把自愈步骤从约 60 行收缩到约 22 行。先提一个注意点:本 PR 有意回退了 #9220 review 轮次(R2–R8)中被接受的意见,那个决定的 owner 应该确认这次反转——无论如何,review 结束时都会按守卫规则交到 maintainer 手里(见最终评论里的说明)。 规模:未触及核心包路径——生产代码 51 行( 方案:范围合理——每处改动要么删除守卫层,要么删除钉住守卫层的测试,没有顺手改动。保留下来的部分( 风险:无高风险路径信号。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI wrote my independent baseline before opening the diff: keep the Things I verified while reading:
No correctness, security, or convention findings. What survives is still pinned by tests that execute the real script: the pool wipe idiom (hidden entries included), the sudo leg with its exact argv, the never-fail exit, the identical retry checkout, and the continue-on-error invariants on all three steps. The author additionally reports three mutation spot-checks red on this branch (sudo leg deleted, Testing — the PR's own CI, via APIThis triage run does not execute PR code (gate rule); the evidence below is the PR's own CI on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The author also notes a pre-existing macOS-lane failure in the #9255 fallback-comment block ( No sandboxed-verification trigger this time, deliberately: the surviving behavior is pinned by real-script-execution tests that CI runs, so there is no behavioural claim left for 中文说明代码审查:我在打开 diff 之前先写下了自己的独立方案——保留 核对过的点:workflow 中 没有正确性、安全性或规范问题。保留的行为仍由执行真实脚本的测试钉住。作者另报告了 3 个变异点检为红(这是作者自己的运行结果)——不过保留的断言确实执行真实脚本,这些变异理应失败。 测试:本次 triage 不执行 PR 代码(门禁规则),以上证据来自 PR 自己的 CI。关键泳道 作者还提到 #9255 fallback-comment 块中一个预存的 macOS 泳道失败,在未改动的 main 上同样失败——本 commit 的 macOS 泳道被跳过,无法从本次 CI 观测,仅作为作者陈述记录。 这次刻意没有给出沙箱验证触发语:保留的行为已由 CI 运行的真实脚本执行测试钉住,没有留给 /verify 的行为性问题;剩下的威胁模型判断是 maintainer 的决策,不是可测试的声明。真实场景测试:N/A(CI 基础设施,无用户可见面)。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review, but this is a fork Stepping back: the threat-model argument is the whole PR, and it holds. My independent proposal matched the PR's shape exactly; I found nothing simpler to keep and nothing removed that the observed failures need. The diff is minimal — every edit serves the stated goal. One line worth quoting when this lands: if the pool ever gains steps that can override the workspace path, a guard belongs back with that change. Why not approving, despite the clean review:
中文说明回顾整体:威胁模型论证是这个 PR 的全部依据,而且成立。 我的独立方案与 PR 形态完全一致:没有更简单的保留形态,也没有删掉任何观测到的失败所需要的东西。diff 是最小的——每处改动都服务于既定目标。有一句话值得在这个 PR 落地时引用:如果未来池上出现能覆写 workspace 路径的步骤,守卫应随那个改动一起加回来。 为什么不批准(尽管 review 是干净的):
— Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to a maintainer for the final call on this one. The review itself is clean (see the stage comments above): the threat-model case for dropping the path-guard layers holds, the surviving wipe-and-retry core stays pinned by real-script-execution tests, and the diff is minimal. Two reasons it still needs a human:
@wenshao — you own both sides of this one (author of this PR and of #9220): please approve as a committer if you stand by the reversal, or loop in another maintainer for an independent look. Needs a human call on this one. 中文说明⏸️ 转交 maintainer 做最终决定。 审查本身是干净的(见上方各阶段评论):删除路径守卫层的威胁模型论证成立,保留的 wipe-and-retry 核心仍由真实脚本执行测试钉住,diff 也是最小化的。仍需要人类介入的两个原因:
@wenshao——这个 PR 的两边都是你(本 PR 与 #9220 的作者):如果确认这次反转,请以 committer 身份批准,或者拉另一位 maintainer 独立看一眼。这件事需要人来拍板。 — Qwen Code · qwen3.8-max |
|
@qwen-code /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. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
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.21.13)
…rkspace wipe Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下: Autofix review round summary — PR #9327Feedback triage
ChangesAdded one test,
The premise was verified to hold at the reviewed commit before editing: zero Red probe (recorded): temporarily inserting a hypothetical override step before the wipe ( No conflict resolution was needed ( Notes
Verification
Commit: 中文说明Autofix 审查轮次总结 — PR #9327反馈分类
变更在
编辑前已验证该前提在被审查 commit 上成立:整个 workflow 中 红测 probe(已记录): 临时在 wipe 之前插入一个假想覆写步骤( 无需解决冲突( 说明
验证
提交: 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
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
@qwen-code /triage |
doudouOUC
left a comment
There was a problem hiding this comment.
Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.
— qwen-code via Qwen Code /review (v0.21.10)
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "本 PR 将 #9220 引入的 checkout 自愈步骤收缩回 wipe-and-retry 核心,删除…": empirical Linux confirmation that find -P does not descend a symlinked starting-point (Windows host could not create a real symlink; relied on documented GNU …; "本 PR 将 #9220 引入的 checkout 自愈步骤收缩回 wipe-and-retry 核心,删除…": none — all planned checks completed within budget. (Note: the suite is excluded on win32 per its own comment, so I verified statically rather than by running vi…; "本 PR 将 #9220 引入的 checkout 自愈步骤收缩回 wipe-and-retry 核心,删除…": could not run scripts/tests/qwen-pr-review-workflow.test.js itself — scripts/tests/vitest.config.ts excludes qwen-*-workflow.test.js on win32 and the work….
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.
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"本 PR 将 #9220 引入的 checkout 自愈步骤收缩回 wipe-and-retry 核心,删除…":empirical Linux confirmation that find -P does not descend a symlinked starting-point (Windows host could not create a real symlink; relied on documented GNU …;"本 PR 将 #9220 引入的 checkout 自愈步骤收缩回 wipe-and-retry 核心,删除…":none — all planned checks completed within budget. (Note: the suite is excluded on win32 per its own comment, so I verified statically rather than by running vi…;"本 PR 将 #9220 引入的 checkout 自愈步骤收缩回 wipe-and-retry 核心,删除…":could not run scripts/tests/qwen-pr-review-workflow.test.js itself — scripts/tests/vitest.config.ts excludes qwen-*-workflow.test.js on win32 and the work…。
未检查(工具限制,非阻断):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.21.10)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下: Autofix review round — PR #9327Round 2 — all 5 automated-reviewer suggestions resolved in code. Each finding was first reproduced on the reviewed commit ( Findings
The test was renamed from Review bodies also read: [rv:4951542859] carries no findings beyond the inline ones; [rv:4953214060] is a statement about that review run's own coverage certification (no plan supplied to it) — it contains no defect claim or actionable suggestion, so no code action applies. Changes
Conflict notes
Verification
中文说明Autofix 审查轮次 — PR #9327第 2 轮——自动审查者的 5 条建议全部已在代码中解决。每条发现均先在被审查 commit( 发现处理
测试由 审查 body 也已阅读:[rv:4951542859] 除行内发现外无其他内容;[rv:4953214060] 是关于该次审查运行自身覆盖率认证的说明(未向其提供 plan)——不含缺陷主张或可执行建议,因此无代码动作。 变更仅 冲突说明
验证
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/模型 |
Addresses the two doudouOUC findings on the simplified heal: - The wipe now validates the filesystem OBJECT at $WS, not just the string: find -P does not descend a symlinked start, so a redirected workspace logged 'wiped for a clean retry' while deleting nothing, and the secret-bearing review step would then run through the redirection. Refuse loud on a symlink or non-directory — POSIX-only, no false-positive surface (a legitimate workspace is always a runner-created plain directory), and it pins the only sudo-escalated wipe in the pool to a validated target. - The clean-wipe silence branch was unpinned: the reviewer's minimal mutant (dropping the if/fi pair) shipped an empty-list survivor warning on every heal with the suite green. The clean-wipe test now asserts the success annotation and the absence of the survivor warning; both mutants verified red.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
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.21.13)
|
🔀 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
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
.github/workflows/qwen-code-pr-review.yml:570 — [review] Trailing-slash spelling bypasses the new object guard
中文说明
已审查——无阻断问题。 建议见行内评论。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 3/100 轮)。改动内容与我反驳保留之处如下: Autofix review round — PR #9327One commit this round: Round-1 findings (@doudouOUC) — fixed in earlier commits, re-verified this round
Round-3/4 findings (automated reviewer) — all addressed in this roundEvery claim below was reproduced on the current code before implementing, and each fix was mutant-verified (mutant applied to the workflow, suite turned red, workflow restored).
Not actionable this round
VerificationCommands actually run this round, in order:
中文说明Autofix 审查轮次 — PR #9327本轮一个 commit: 第 1 轮发现(@doudouOUC)——已在先前 commit 修复,本轮复核
第 3/4 轮发现(自动审查器)——本轮全部处理以下每项都在实施前于当前代码上复现,且每个修复都经过变异验证(变异施加到 workflow → 套件变红 → workflow 逐字节还原)。
本轮不处理
验证本轮实际执行的命令,按顺序:
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
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: the executable-script lint — qwen review script-lint produced no report.
Not explored to full depth (tool budget reached): "You are review agent 6b — Agent 6b: Undirected audit — 3…": None. I read the full diff, examined both the old and new versions of the workflow file and test file, verified the continue-on-error wiring on the first chec…; "You are review agent 6c — Agent 6c: Undirected audit —…": none (within the ~36-call budget).; "You are review agent 6a — Agent 6a: Undirected audit —…": None. All checks completed within budget..
Not reviewed: verification and reverse audit — neither the verifier nor the reverse auditor was launched with a prompt this skill builds — the posted findings were ruled on, and the misses the rest of the review left were hunted, if at all, without the briefs this skill certifies against.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:the executable-script lint — qwen review script-lint produced no report。
未探索到全部深度(达到工具调用预算):"You are review agent 6b — Agent 6b: Undirected audit — 3…":None. I read the full diff, examined both the old and new versions of the workflow file and test file, verified the continue-on-error wiring on the first chec…;"You are review agent 6c — Agent 6c: Undirected audit —…":none (within the ~36-call budget).;"You are review agent 6a — Agent 6a: Undirected audit —…":None. All checks completed within budget.。
未审查:验证与反向审计——验证 agent 与反向审计 agent 都没有用本 skill 构建的 prompt 启动——发布的发现即便被裁定过、评审其余部分遗漏的问题即便被搜寻过,也都缺失了本 skill 用以认证的 brief。
— qwen3.8-max via Qwen Code /review (v0.21.10)
doudouOUC
left a comment
There was a problem hiding this comment.
PR head 已漂移
审查锁定的 head SHA 为 1b33a9abf8fa9199d376c10ff98084dd9268ccd6,当前 PR head 已变为 95391df97f8f2317f4418ac515fa39cbb111d50d。原 inline 评论可能已失效,以下仅保留审查汇总。
审查汇总(deepseek-v4-flash)
PR #9327 将 #9220 引入的 checkout 自愈步骤从约 60 行守卫层 + 13 个测试收缩为约 22 行 wipe-and-retry 核心 + 7 个测试,净 −294 行。
结论:无阻塞问题,但识别出 5 条改进建议。
主要发现:
qwen-code-pr-review.yml:562— 新守卫未检查$WS是否为系统根目录(/、/usr等)。runner 配置错误时可能通过 symlink/目录检查并进入sudo -n findwipe。qwen-pr-review-workflow.test.js:2810—[ ! -d "$WS" ]守卫分支(路径不存在)缺少专属测试,仅测试了 symlink 分支。qwen-pr-review-workflow.test.js:2758— “could not wipe the workspace” else 分支消息未断言,消息漂移无法被现有测试捕获。qwen-pr-review-workflow.test.js:2847— seal 测试只扫描step.run文本中的$GITHUB_ENV/$GITHUB_PATH,未检查uses:action 步骤;当前无风险但建议文档化。qwen-code-pr-review.yml:568— 非 sudo 的find命令使用2>/dev/null静默丢弃错误信息,不利于调试。
总体判断:删除守卫层的威胁模型论据成立;保留的测试仍能覆盖核心事故场景。
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
.github/workflows/qwen-code-pr-review.yml:581 — [probe] Survivor warning dropped the entry count while keeping the 500-char truncation — a truncated list presents itself as complete (unchanged since round 4; deferred under the convergence p…
中文说明
已审查——无阻断问题。 建议见行内评论。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Addresses the open review findings on the simplified heal: - The seal's premise covered declarative env, $GITHUB_ENV/$GITHUB_PATH run writes, and the pre-wipe action set, but three channels passed it unchecked: a wipe-step `shell:` or workflow/job `defaults:` wrapper re-targets the environment at exec time; SHELLOPTS rides the same bash-startup family as BASH_ENV/ENV yet sat outside the dangerous name class; and ACTIONS_ALLOW_UNSECURE_COMMANDS re-enables the legacy ::set-env:: / ::add-path:: spellings the run-text scan did not match. Each channel was reproduced green against the old seal (mutant probe) and now turns it red. - The both-legs-fail test now also pins the else-branch "could not wipe" warning, and a dedicated test pins the `[ ! -d ]` refusal for a nonexistent workspace — the plain-file test alone still passes a guard mutated to `[ -f ]`. - The non-sudo wipe leg keeps its stderr: the 2>/dev/null discarded exactly the diagnostics oncall needs when the wipe fails, and the sudo leg already ran unsuppressed.
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 4/100 轮)。改动内容与我反驳保留之处如下: Autofix round summary — PR #9327Implemented 4 of the 6 inline findings and declined 2 with recorded evidence. No conflict resolution was needed ( FindingsImplemented
Declined (with evidence)
Conflict notesNone — Verification
中文说明Autofix 轮次总结 — PR #9327本轮实现了 6 条行内发现中的 4 条,另有 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/模型 |
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.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
.github/workflows/qwen-code-pr-review.yml:555 (+3 locations) — [probe] pool-wipe siblings (qwen-triage.yml before/after external code, serve-ab.yml) lack the object check this diff adds and names as one idiomscripts/tests/qwen-pr-review-workflow.test.js:2861 — [review] R5-1 premise-seal exhaustiveness claim still stands: round-5 entrances fixed at this head, new ${{ }} interpolation channel demonstrated this round
中文说明
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
🔀 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
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 10.
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 7, not a blocker) — recorded, not requested in this round:
scripts/tests/qwen-pr-review-workflow.test.js:2861 — [probe] R5-1 class (round 5, still standing): premise-seal exhaustiveness claim unsatisfiable — round-7 entrances probe-verified: indirect $(printenv GITHUB_PATH)/${!v} spellings; NODE_OP…scripts/tests/qwen-pr-review-workflow.test.js:2899 — [probe] RA4-1: seal's ::set-env:: leg is dead — regex requires '::set-env::' but the only valid syntax is '::set-env name=VAR::'; fix /::(set-env|add-path)\b/ (probe flips green→red)scripts/tests/qwen-pr-review-workflow.test.js:2886 — [probe] RA5-1: seal's dangerousEnv scan never inspects pre-wipe steps' own step-level env blocks — step-local BASH_ENV executes planted code whose $GITHUB_PATH write persists (probe-verif…
中文说明
仅完成部分审查,审查缺口已披露。
未审查:反向审计——在 10 轮的反审轮数上限内未收敛。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 7 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 5/100 轮)。改动内容与我反驳保留之处如下: Address-review summary — PR #9327Feedback triage
Both inline Criticals describe the same defect: the replacement guard Reproduction (before fixing)I reproduced the defect on the current head with a standalone probe that runs the REAL extracted wipe script ( I also added the regression test first and confirmed it FAILED on the unfixed code (1 failed | 11 passed), proving the test pins a real defect rather than an already-green path. FixMinimal, root-cause, exactly the reviewer's suggested change: validate the resolved location, not just the last component. A runner-created workspace has no symlink components at all, so requiring if [ "$(realpath -- "$WS")" != "$WS" ] || [ -L "$WS" ] || [ ! -d "$WS" ]; then
echo "::error::workspace is not a plain directory or resolves through symlinks: $WS"
exit 1
fi
Post-fix probe + testsThe full Notes
VerificationCommands actually run (results):
No settings source changed, so 中文说明处理评审总结 — PR #9327反馈分类
两条 inline Critical 描述的是同一个缺陷:替换后的守卫 复现(修复前)我在当前 head 上用一个独立 probe 复现了该缺陷:该 probe 运行真实提取出的 wipe 脚本( 我还先添加了回归测试并确认它在未修复代码上失败(1 failed | 11 passed),证明该测试确实钉住了一个真实缺陷,而不是一条本来就绿的路径。 修复最小化、根因修复,与评审者建议完全一致:校验解析后的位置,而非仅最后一个分量。runner 创建的 workspace 完全不含符号链接分量,因此要求 if [ "$(realpath -- "$WS")" != "$WS" ] || [ -L "$WS" ] || [ ! -d "$WS" ]; then
echo "::error::workspace is not a plain directory or resolves through symlinks: $WS"
exit 1
fi
修复后的 probe + 测试完整的 说明
验证实际运行的命令(结果):
未改动任何 settings 源文件,因此无需运行 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 review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base:红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": locating the runner's exact github-context→step-process-env translation site that decides whether a $GITHUB_ENV write of GITHUB_WORKSPACE survives into the ne….
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 8, not a blocker) — recorded, not requested in this round:
.github/workflows/qwen-code-pr-review.yml:573 — [review] wipe guard now exists as four divergent copies across the pool; manual porting has a documented miss (#9265) — port the containment guard to the siblings or extract a shared wipe scri…scripts/tests/qwen-pr-review-workflow.test.js:2884 — [probe] nonexistent-workspace test comment misattributes the refusing disjunct — realpath fails first for the deep-missing fixture; the claimed [ ! -d ]→[ -f ] mutation is not caught by t…scripts/tests/qwen-pr-review-workflow.test.js:2936 — [probe] R5-1 class (round 5, still standing): round-8 entrances probe-verified — seal scan misses indirect GITHUB_ENV/GITHUB_PATH writes ($(printenv), ${!v}, backticks, script delegation)…
中文说明
无阻断问题。LGTM!✅
未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 4)":locating the runner's exact github-context→step-process-env translation site that decides whether a $GITHUB_ENV write of GITHUB_WORKSPACE survives into the ne…。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 8 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: Autofix round: no actionCritical-only mode is active for PR #9327 (5 change-producing rounds are complete in this counting window). Every actionable section for this round is empty:
The only newer feedback is a non-Critical automated-reviewer PR comment, which the workflow's deterministic brake excluded from this round and deferred for human follow-up. Per the Critical-only rules, no code changes were made, no threads were resolved, and no comment replies were written for deferred items. No commits were made this round; the PR head remains at 中文说明PR #9327 已进入 Critical-only 模式(本计数窗口内已完成 5 个产生改动的轮次)。本轮所有可执行区域均为空:
本轮唯一更新的反馈是一条来自自动审查器的非 Critical PR 评论,已被工作流的确定性制动机制从本轮排除,延后留待人工跟进。按照 Critical-only 规则,本轮未做任何代码改动、未解析任何线程、也未对延后条目撰写评论回复。 本轮未产生任何提交;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 无冲突。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
There was a problem hiding this comment.
Approved. The R6-1 Critical (wipe guard only inspects the final path component) is addressed at the head: the guard now validates realpath -- "$WS" against $WS plus the -L/-d checks, matching the suggested fix exactly. CI green on the head (Test, secret scan, CVE audit, Classify PR). The remaining open threads are Suggestion-level (test-pin coverage, comment accuracy) and non-blocking.
doudouOUC
left a comment
There was a problem hiding this comment.
Approved. CI-infrastructure-only change (2 files, +158/-298); reviewed at b7a549e7245f491a8bda7fd4b6c4214d13b05a39 against base d02e252d41.
Verified by execution, not by reading
- Ran the suite in an isolated detached worktree at the head:
npx vitest run --config ./scripts/tests/vitest.config.ts qwen-pr-review-workflow.test.js→ 155 passed, 1 failed (156). - The single failure is
fallback comment resilience (PR #8894 incident class) > repairs a single unwritable directory instead of failing fast. I re-ran that same test on the based02e252d41and it fails identically there. So it is pre-existing and untouched by this diff, exactly as the PR body discloses — not a regression introduced here.
Guard ordering traced literally at the reviewed commit
WS="${GITHUB_WORKSPACE:?}" (yml 571) runs first, then the refusal at yml 572-575 — [ "$(realpath -- "$WS")" != "$WS" ] || [ -L "$WS" ] || [ ! -d "$WS" ] → ::error:: + exit 1 — lands before both wipe legs and before the sudo -n leg (yml 576). A workspace redirected through a symlinked intermediate component, a non-directory, or a nonexistent path therefore cannot reach the destructive command, and the sudo leg (the pool's only root-deletion power) only ever operates on a validated plain directory. The step carries no continue-on-error, so both deliberate nonzero exits fail the job loud rather than degrading to a log annotation.
The claimed-surviving behaviors are pinned by tests that actually executed
The pool wipe idiom including hidden entries, the sudo fallback leg with its exact argv, the never-fail exit contract with survivors named in the warning, the identical retry checkout (retry.uses/retry.with compared field-for-field against the first), and the continue-on-error invariants across all three steps — each has a passing test at this commit. seals every override channel into the wipe step is the load-bearing one: with the allowlist gone it enforces the PR's threat-model premise inside the workflow file, checking dangerous env: entries by name class rather than by enumeration, both bare and braced $GITHUB_ENV/$GITHUB_PATH spellings, the legacy ::set-env::/::add-path:: forms, the pre-wipe uses: set, and the shell selection. That is the right shape for this — it closes the channel a string guard inside the script could never reach.
One correction to the PR description (non-blocking, no behavior impact)
The stated test counts are stale relative to the head — they describe an earlier commit, before the review rounds added tests back. Measured at b7a549e7: the suite goes 157 → 156 (not 157 → 151) and the checkout self-heal block goes 13 → 12 (not "now 7 tests"). Net −294 lines is still accurate. Worth correcting so the next reader of this description is not misled about how much test coverage was dropped.
On the removed denylist/allowlist
I did not re-open this. It is a deliberate, argued scope decision already carried in the open threads, and the seal test enforces the premise it rests on. Recording the residual only for the record: the premise is enforced by a test, not by the script, so if that seal is ever weakened, a mangled GITHUB_WORKSPACE becomes reachable again (realpath -- / resolves to itself, so a bare / would pass the object check). The PR body's own note — that a guard belongs back the moment a step that can write the runner env file appears — is the correct trigger condition, and the seal test is what will detect it.
CI: 16 success, 50 skipped, 0 failures. No blockers from me.
中文说明
已批准。纯 CI 基础设施改动(2 个文件,+158/−298);审查基于 b7a549e7245f491a8bda7fd4b6c4214d13b05a39,基线 d02e252d41。
执行验证(非仅阅读)
- 在头提交的隔离 detached worktree 中运行测试套件:155 通过,1 失败(共 156)。
- 唯一失败项为
repairs a single unwritable directory instead of failing fast。我在基线d02e252d41上单独重跑该测试,同样失败。因此这是既有失败、与本 diff 无关,与 PR 描述的说明一致,并非本次引入的回归。
守卫顺序(逐行核对)
WS="${GITHUB_WORKSPACE:?}"(yml 571)先执行,随后 yml 572-575 的拒绝分支 —— [ "$(realpath -- "$WS")" != "$WS" ] || [ -L "$WS" ] || [ ! -d "$WS" ] → ::error:: + exit 1 —— 位于两条 wipe 分支与 sudo -n 分支(yml 576)之前。因此经中间符号链接重定向的 workspace、非目录、不存在的路径都无法触达破坏性命令,sudo 分支(池上唯一的 root 删除权限)只会作用于已校验的普通目录。该步骤没有 continue-on-error,两个故意的非零退出都会让 job 响亮失败。
PR 声称保留的行为均由实际执行通过的测试钉住
含隐藏条目的池 wipe 惯用法、带精确 argv 的 sudo 兜底分支、点名残留项的永不失败退出契约、逐字段比对的一致 retry checkout、三个步骤的 continue-on-error 不变式,在该提交上都有通过的测试。其中 seals every override channel into the wipe step 是承重项:在 allowlist 移除后,它把 PR 的威胁模型前提固化在 workflow 文件内部,按名称类而非枚举检查危险 env: 条目,覆盖 $GITHUB_ENV/$GITHUB_PATH 的裸写与花括号写法、legacy ::set-env::/::add-path::、wipe 前的 uses: 集合以及 shell 选择。这个形状是对的——它封住了脚本内字符串守卫根本触达不到的通道。
对 PR 描述的一处更正(非阻断,不影响行为)
描述中的测试数量相对头提交已过期,反映的是 review 轮次补回测试之前的某个更早提交。在 b7a549e7 上实测:套件总数为 157 → 156(非 157 → 151),checkout self-heal 块为 13 → 12(非"现在 7 个测试")。净 −294 行仍然准确。建议修正,以免后续读者误判被删掉的测试覆盖量。
关于被移除的 denylist/allowlist
我没有重新开启这个议题。这是已在现有讨论线程中充分论证过的有意范围决策,且 seal 测试守住了它所依赖的前提。仅记录残余风险:该前提由测试而非脚本强制,因此一旦该 seal 被削弱,畸变的 GITHUB_WORKSPACE 会重新变得可达(realpath -- / 解析为自身,裸 / 能通过对象检查)。PR 描述自己给出的触发条件——一旦出现能写 runner env 文件的步骤就应把守卫加回——是正确的判断标准,而 seal 测试正是发现它的手段。
CI:16 成功,50 跳过,0 失败。我这边无阻断问题。
|
Released in v0.21.14. |
What this PR does
Shrinks the checkout self-heal added in #9220 back to its load-bearing core. Eight review rounds grew the heal step from about 15 lines into about 60 lines of path-guard layers — realpath canonicalization, two trailing-slash strip loops, a denylist, and a runner-workspace allowlist — plus roughly 450 lines of tests pinning their mutation resistance. This PR removes all of those guard layers and their tests, keeping the wipe-and-retry core: net −294 lines. What survives is still pinned by tests: the pool wipe idiom (including hidden entries), the sudo fallback leg with its exact argv, the never-fail exit contract with survivors named in the warning, the identical retry checkout, and the continue-on-error invariants on all three steps.
Why it's needed
The heal exists for two observed, realistic failure shapes: a corrupt persisted workspace (seven review jobs dead on the same missing SHAs on one runner, 2026-08-13..15) and a transient fetch drop. Both are still fully covered. Everything removed defended against a different shape — a mangled workspace path handed to the wipe. That variable is set by actions/runner itself; the only actors that could mangle it are a compromised runner or a workflow step overriding it, and both already execute arbitrary code on the machine and need no wipe to do damage. A guard inside a script cannot defend against the actor that controls the script's host, so the removed layers carried maintenance and portability cost (a GNU-only realpath flag, a BSD fallback, a host-probe test axis) while defending against nothing realistic. The one guard that does earn its place — failing loud when the variable is dropped entirely — stays, and stays tested.
Reviewer Test Plan
How to verify
The happy path is byte-identical in effect: a failed first checkout still wipes the workspace contents (directory kept), still escalates to passwordless sudo where available, still warns which shape happened, still names any survivors, and still retries the identical checkout. Run the workflow test suite: the checkout self-heal block is now 7 tests (was 13) and all pass; the suite total goes 157 → 151. Three spot mutants were verified red on this branch: deleting the sudo fallback leg, dropping the
:?guard, and addingcontinue-on-errorto the wipe step each fail the corresponding test. The pre-existing macOS failure in the #9255 fallback-comment block (repairs a single unwritable directory) is unchanged by this PR — it fails identically on untouched main.Evidence (Before & After)
N/A — CI infrastructure change, no user-visible output. Before: a 60-line heal step with four guard layers and 19 tests, of which 12 existed to pin guard mutation resistance. After: a 22-line step and 7 behavior tests; the incident it was built for (corrupt workspace → wipe → clean retry) is exercised by the same real-script execution tests as before.
Tested on
Environment (optional)
Unit tests only (vitest executing the real wipe script under runner shell flags), plus the three mutation spot-checks above. Removing the GNU-only realpath line also removes the test axis that had to probe for it, so the suite is now host-independent.
Risk & Scope
Linked Issues
None — follow-up to #9220, observed via its review-loop history.
中文说明
这个 PR 做了什么
把 #9220 引入的 checkout 自愈步骤收缩回承重核心。经过 8 轮 review,这个自愈步骤从约 15 行膨胀到约 60 行路径守卫层——realpath 规范化、两条尾斜杠剥离循环、一个黑名单、一个 runner-workspace 白名单——外加约 450 行钉住它们变异抗性的测试。本 PR 删掉全部守卫层及其测试,保留 wipe-and-retry 核心:净 −294 行。留下来的行为仍被测试钉住:池 wipe 惯用法(含隐藏条目)、带精确 argv 断言的 sudo 兜底分支、命名残留项的永不失败退出契约、完全一致的 retry checkout、以及三个步骤上的 continue-on-error 不变式。
为什么需要
自愈机制面向两种观察到的真实失败形态:损坏的持久化 workspace(一台 runner 上连续 7 个 review job 死于同一批缺失 SHA,2026-08-13..15)和瞬时 fetch 中断。这两种形态仍然完整覆盖。被删除的部分防的是另一种形态——畸变的 workspace 路径被交给 wipe。这个变量由 actions/runner 自己设定;能畸变它的只有被入侵的 runner 或覆写它的 workflow 步骤,而这两者都已经在机器上拥有任意代码执行权,不需要 wipe 就能造成破坏。脚本内的守卫防不住控制脚本宿主的行为者,所以被删的层只带来维护和可移植性成本(GNU 专属的 realpath 标志、BSD 回退、host-probe 测试轴),却没有防御任何现实威胁。唯一值得保留的守卫——变量整个丢失时响亮失败——留下了,并且仍有测试。
Reviewer 测试计划
如何验证
正常路径的效果逐字节等价:第一次 checkout 失败后仍会清空 workspace 内容(保留目录本身)、仍在有免密 sudo 的机器上提权重试、仍会警告发生了哪种情况、仍会点名残留项、仍会用完全一致的参数重试 checkout。运行 workflow 测试套件:checkout self-heal 块现在是 7 个测试(原 13 个),全部通过;套件总数 157 → 151。本分支上验证过 3 个点状变异全部变红:删除 sudo 兜底分支、去掉
:?守卫、给 wipe 步骤加continue-on-error,各自对应的测试都会失败。#9255 fallback-comment 块里那个预存的 macOS 失败(repairs a single unwritable directory)与本 PR 无关——在未改动的 main 上同样失败。Before / After 证据
N/A——CI 基础设施改动,无用户可见输出。Before:60 行自愈步骤、四层守卫、19 个测试,其中 12 个用于钉守卫的变异抗性。After:22 行步骤、7 个行为测试;它为之而生的事故场景(损坏 workspace → wipe → 干净重试)仍由同一套真实脚本执行测试覆盖。
测试平台
环境(可选)
仅单元测试(vitest 以 runner shell 标志执行真实 wipe 脚本),加上上述 3 个变异点检。删除 GNU 专属的 realpath 行也删掉了需要探测它的测试轴,套件从此与宿主无关。
风险与范围
关联事项
无——#9220 的后续,源自其 review 循环历史的观察。