fix(ci): move undeletable workspace residue aside instead of leaving it - #9868
Merged
Merged
Conversation
A leftover the pre-checkout sweep cannot delete does not just fail the job that finds it — it poisons the checkout of every later job scheduled onto that runner. Measured on run 32621267802: residue from a review probe (`.qwen/tmp/review-pr-9748-scratch-verify-…/probe-ws/.qwen/tmp/review-pr-666`) survived on one shared-pool member, and two unrelated PRs then died at Checkout with the same EACCES on the same path. The sweep's own steps had reported success: their ladder ends at a warning, so a workspace it cannot repair is handed to actions/checkout unchanged. The recovery the ladder was missing is a rename. Unlinking an entry needs write permission on the directory holding it — exactly what foreign-owned residue denies on a pool member without passwordless sudo — while renaming needs it only on the two parents, and the workspace root is always the runner's own. So a tree that defeats rm, chmod, and chown still moves out of the way, and the checkout finds nothing to trip on. The destination sits next to the workspace so the rename cannot cross a filesystem and degrade into copy-then-unlink, and the warning names it, because the tree still needs a human. Reproduced in a Linux container with real foreign ownership (residue owned by root, workspace by the runner user, sudo unavailable): before, the step warns `leaked .qwen`, the tree stays, and a checkout-style wipe fails with `Permission denied`; after, the tree is quarantined, the wipe succeeds, and the warning points at where it went.
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献！
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewThe approach matches what I'd have proposed from the problem statement alone: unlinking needs write permission on the directory holding each entry — exactly what foreign-owned residue denies — while renaming needs it only on the two parents, and both the workspace root and the newly created quarantine directory are the runner's own. So a tree that defeats
No blockers. Two non-blocking notes: (1) the net growth lands ci.yml exactly at the size-ratchet allowance (4,096 bytes over baseline — zero headroom, so the next ci.yml edit needs a Test evidence — the PR's own CI via the API (PR code not executed locally)No failures at fetch time. The macos/windows Test jobs and Integration Tests are skipped by design ( The sweep-step behaviour itself is covered by that suite: the tests run the extracted step body on ubuntu (non-root, bash available), and the mode-500 fixture exercises the same Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查这个方案与我只看问题描述时会给出的方案一致：删除（unlink）需要每个条目所在目录的写权限——而外部属主残留恰恰拒绝这一点；改名（rename）只需要两端父目录的写权限，而工作区根目录和新建的隔离目录都是 runner 自己的。因此能扛住
无阻塞项。两条非阻塞提醒：(1) 净增长恰好把工作流大小顶到棘轮允许值（超基线 4,096 字节——零余量，下次修改 ci.yml 需要更新 测试证据——经 API 读取 PR 自身 CI（未在本地执行 PR 代码）抓取时无失败项。macos/windows 的 Test 与集成测试按设计跳过（仅 清理步骤本身的行为由该套件覆盖：测试在 ubuntu（非 root、bash 可用）上执行提取出的步骤体，mode-500 夹具触发与事故相同的 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — a clean, well-evidenced CI reliability fix; the only nits are cosmetic (zero-headroom size ratchet) and an acknowledged tradeoff (quarantine accumulation). What convinced me: the problem is real — I verified it independently against the original run (PR #9776's attempt 1 died at Checkout with every downstream job skipped), not just taken from the PR's word. The fix is the smallest thing that could work: renaming is the right last resort when deletion is denied, and the chain stays a strict superset of the old behavior, so nothing that worked before changes. The tests aren't decorative either — they execute the real step body from ci.yml against a fixture reproducing the incident's EACCES failure, and both the static pin and the existence assertion are provably red against the old step body, so the suite pins this change. Remaining notes are non-blocking: the ci.yml growth lands exactly at the size-ratchet allowance (zero headroom — the next ci.yml edit pays the baseline bump), and the quarantine directory will occasionally need a human. Both flagged in the review. Approval deferred until CI lands green on the reviewed commit — the ubuntu Test job carrying the new tests is still in flight. The finalize workflow posts the commit-pinned approval if everything comes back green, and flags the status comment if anything lands red. 中文说明Confidence: 4/5 —— 干净、证据充分的 CI 可靠性修复；仅有的小问题是外观性的（大小棘轮零余量）和一个已声明的取舍（隔离目录累积）。 说服我的地方：问题是真实的——我对照原始 run 独立核实过（PR #9776 的第 1 次尝试在 Checkout 步骤失败、后续所有 job 被跳过），而不是只听 PR 自述。修复是能起作用的最小改动：删除被拒绝时，rename 是正确的最后手段，且链条仍是旧行为的严格超集，之前能工作的行为完全不变。测试也不是摆设——它们执行从 ci.yml 提取的真实步骤体，夹具复现了事故的 EACCES 失败；静态锁定和存在性断言对旧步骤体都可证为红，因此套件锁定了本变更。 其余提醒均为非阻塞：ci.yml 的增长恰好顶到大小棘轮允许值（零余量——下次修改 ci.yml 要付出基线更新的代价），隔离目录偶尔需要人工清理。两者都已在审查中指出。 审批推迟到 CI 在被审提交上变绿——承载新测试的 ubuntu Test job 仍在运行。全部变绿后，finalize 工作流会发布锁定到该提交的审批；若有变红，会在状态评论中标出。 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
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
approved these changes
Aug 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Collaborator
Author
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
Collaborator
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Comment on lines
+157
to
+158
| // the rename fails. | ||
| expect(code).toContain('leaked .qwen; runner needs manual cleanup'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The rename-failure branch of the new quarantine fallback (the else of if mv ...) has no behavioral test — its only pin is the substring check above, and the two behavioral fixtures exercise only rm-success and quarantine-success; nothing ever makes mv fail. A mutant that preserves the warning string but lets the else path return non-zero (e.g. appending && rmdir "$GITHUB_WORKSPACE/.qwen" to the else echo, or dropping || true from mkdir -p) survives all 15 tests — confirmed by probe: with that mutant applied to all three ci.yml copies the whole suite still passes. On a runner where the rename is impossible, the brace group then exits non-zero under the runner's default bash -e and fails the pre-checkout step before checkout — re-creating the exact "cleanup blocks the job" shape this PR exists to fix.
Witness (probe in an isolated tree): the missing fixture — mirroring the undeletable-residue test with the quarantine parent locked (chmod 0o500) so mkdir -p and mv both fail — fails against the mutant (AssertionError: expected 1 to be +0 under set -euo pipefail) and passes on the PR code, while all 15 existing tests stay green on the mutant.
Suggested fix: add that third behavioral fixture — assert out.status === 0, .qwen still present in the workspace, no _qwen-quarantine entry created, and exactly one ::warning:: containing leaked .qwen; runner needs manual cleanup; repair permissions in finally with the same pattern test 1 already uses.
中文说明
新增隔离兜底的「重命名失败」分支（if mv ... 的 else）没有行为测试——它唯一的锁定就是上面的子串检查，而两个行为夹具只覆盖了 rm 成功与隔离成功两种情形；mv 从未被置于失败境地。一个保留警告字符串但让 else 路径返回非零的变异（例如在 else 的 echo 后追加 && rmdir "$GITHUB_WORKSPACE/.qwen"，或去掉 mkdir -p 后的 || true）能通过全部 15 项测试——探针已证实：把该变异应用到 ci.yml 的三份副本后，整个测试套件仍然全绿。而在一台连重命名都做不到的 runner 上，花括号分组会在 runner 默认的 bash -e 下以非零退出，导致 checkout 前的清理步骤在 checkout 之前失败——恰恰重现了本 PR 要修复的「清理挡住 job」的事故形态。
证据（隔离树中的探针）：补上缺失的夹具——仿照「不可删除残留」测试、但把隔离目录的父目录锁成 0o500，使 mkdir -p 与 mv 双双失败——它对变异体失败（set -euo pipefail 下 AssertionError: expected 1 to be +0），对 PR 原代码通过；同时现有 15 项测试在变异体上依旧全绿。
建议修复：补上这第三个行为夹具——断言 out.status === 0、工作区中 .qwen 仍在、未创建任何 _qwen-quarantine 条目、且恰好一条包含 leaked .qwen; runner needs manual cleanup 的 ::warning::；finally 中沿用测试 1 已有的权限修复模式。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // unconditional relocation: a workspace that deletes cleanly keeps its | ||
| // caches. | ||
| expect(code).toMatch( | ||
| /rm -rf "\$GITHUB_WORKSPACE\/\.qwen"[\s\S]*?sudo -n rm -rf[\s\S]*?mv -- "\$GITHUB_WORKSPACE\/\.qwen"/, |
Collaborator
There was a problem hiding this comment.
[Suggestion] This chain-order pin checks the sudo rung's verb only (sudo -n rm -rf), not its target path — and the one behavioral test that reaches the fallback stubs sudo() { return 1; }, so the argument is never observed. Confirmed by probe: the one-line mutation sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" → sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen.bak" survives this regex and both behavioral tests (all 15 pass), and an argument-recording sudo stub shows the deletion rung then targeting a nonexistent path. On pool members with passwordless sudo, foreign-owned residue would silently never be deleted again — every incident degrades to quarantine-plus-manual-cleanup instead of a clean deletion, and nothing in the suite goes red.
Suggested change
| /rm -rf "\$GITHUB_WORKSPACE\/\.qwen"[\s\S]*?sudo -n rm -rf[\s\S]*?mv -- "\$GITHUB_WORKSPACE\/\.qwen"/, | |
| /rm -rf "\$GITHUB_WORKSPACE\/\.qwen"[\s\S]*?sudo -n rm -rf "\$GITHUB_WORKSPACE\/\.qwen"[\s\S]*?mv -- "\$GITHUB_WORKSPACE\/\.qwen"/, |
中文说明
这条顺序锁定只检查了 sudo 阶梯的动词（sudo -n rm -rf），没有检查它的目标路径——而唯一会走到兜底分支的行为测试又把 sudo 桩成 sudo() { return 1; }，因此该参数从未被观测到。探针已证实：单行变异 sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" → sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen.bak" 能通过该正则与两个行为测试（15 项全绿）；用记录参数的 sudo 桩可看到删除阶梯随后指向一个不存在的路径。在配备免密 sudo 的池节点上，异属主残留将悄无声息地永远删不掉——每次事故都退化为「隔离 + 人工清理」而非干净删除，而套件中没有任何测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.0)
yiliang114
approved these changes
Aug 24, 2026
Collaborator
|
Released in v0.22.2. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Gives the pre-checkout sweep in
ci.ymla last resort for residue it cannot delete: move the tree out of the workspace instead of warning and handing it toactions/checkoutunchanged.The recovery is a rename, and that is the whole idea. Unlinking an entry needs write permission on the directory holding it — exactly what foreign-owned residue denies on a pool member without passwordless sudo — while renaming needs it only on the two parents, and the workspace root is always the runner's own. So a tree that defeats
rm,chmod, andchownstill moves aside. The destination sits beside the workspace so the rename cannot cross a filesystem and silently degrade into copy-then-unlink, and the warning names it, because the quarantined tree still needs a human. Removal stays the normal path: a workspace that deletes cleanly keeps its caches and prints nothing.Why it's needed
A leftover the sweep cannot delete does not just fail the job that finds it — it poisons the checkout of every later job scheduled onto that runner.
Measured on run 32621267802: residue from a review probe survived on one shared-pool member —
— and two unrelated PRs (#9776 and
feat/review-coverage-ledger) then died at Checkout on the identical runner and the identical path. Both pre-checkout steps had reported success on the way there: their ladder ends atecho "::warning::leaked .qwen; runner needs manual cleanup", so a workspace they cannot repair is passed to checkout exactly as found. The runner stays poisoned until someone logs into the host.The existing ladder already handles what it can —
chmod, thensudo -n chown/chmodwhere the member has it. This adds the rung for the case those cannot reach, which is the case that was actually observed.Reviewer Test Plan
How to verify
npx vitest run scripts/tests/review-worktree-cleanup-workflow.test.js— 15 pass, including three new ones. Two execute the real step body extracted fromci.ymlagainst a fixture: one where the residue cannot be unlinked (the incident), one where it can (the fallback must stay a fallback).Before (
origin/main's step):After (this branch):
actionlintand the workflow-size gate pass.Evidence (Before & After)
Before: an unrepairable workspace is handed to
actions/checkout, which fails; the runner keeps failing every job scheduled onto it.After: the same workspace is cleared by rename, the checkout proceeds, and a warning names the quarantine directory for manual cleanup. Both states are shown above from the container reproduction, and both are pinned by tests that fail against the previous step body.
Tested on
Unit suite on macOS; the foreign-ownership reproduction in a Debian container. The steps run only on the Linux self-hosted pool.
Environment (optional)
Node.js 22 development checkout on macOS, Docker for the Linux fixture.
actionlintand.github/scripts/check-workflow-size.shboth pass.Risk & Scope
_qwen-quarantinenext to the workspace, since by definition this job cannot delete them. That is the point — they are out of the checkout path — but the directory does need occasional manual cleanup, and the warning says so. It is only ever written on a runner that would otherwise have broken.remove_review_treeladder ends in the same warn-and-leave rung and could take the same fallback; that is a follow-up. This PR fixes the universal path — the pre-checkout sweep every shared-pool job runs — so residue from any workflow stops breaking checkouts. It does not stop residue being created.Linked Issues
None. Observed on run 32621267802 (#9776 and
feat/review-coverage-ledgerboth failing at Checkout on runner…-18).中文说明
本 PR 做什么
给
ci.yml的 checkout 前清理步骤加一条最后手段：删不掉的残留改为移出工作区，而不是打个警告就把工作区原样交给actions/checkout。这条恢复手段是 rename，而这正是要点所在。删除一个条目需要其所在目录的写权限——在没有免密 sudo 的机器上，异属主残留恰恰不给这个权限——而重命名只需要两个父目录的写权限，且工作区根始终归 runner 自己所有。因此一棵能挫败
rm、chmod、chown的树，仍然可以被挪开。目标位置紧邻工作区，使重命名不会跨文件系统而悄悄退化为「复制后删除」；警告会点名该位置，因为被隔离的树仍然需要人工处理。删除仍是正常路径：能干净删除的工作区保留其缓存，且不输出任何内容。为什么需要
清理步骤删不掉的残留，不只是让发现它的那个 job 失败——它会污染之后被调度到那台 runner 的每一个 job 的 checkout。
实测于 run 32621267802：一次评审探针的残留留在了共享池的某台机器上——
——随后两个互不相关的 PR（#9776 与
feat/review-coverage-ledger）在同一台 runner、同一路径上死在 Checkout。而两个 checkout 前步骤一路都报告成功：它们的阶梯止于echo "::warning::leaked .qwen; runner needs manual cleanup"，因此修不好的工作区被原封不动交给了 checkout。这台 runner 会一直被污染，直到有人登录主机处理。现有阶梯已经处理了它能处理的部分——
chmod，以及在有权限的机器上sudo -n chown/chmod。本 PR 补的是那些手段够不到的情形，也正是实际观察到的情形。评审测试计划
如何验证
npx vitest run scripts/tests/review-worktree-cleanup-workflow.test.js——15 项通过，其中 3 项是新增。两项会执行从ci.yml中抽出的真实步骤体：一项针对无法删除的残留（即事故形态），一项针对可以删除的残留（兜底必须始终只是兜底）。leaked .qwen、树仍在、checkout 式清理以Permission denied失败；修复后树被隔离、清理成功、警告点明去向。actionlint与 workflow 体积门通过。证据（前后对比）
前：修不好的工作区被交给
actions/checkout，checkout 失败；该 runner 之后每个 job 都会继续失败。后：同一个工作区通过 rename 被清空，checkout 得以继续，并以警告点名隔离目录供人工清理。两种状态均来自上述容器复现，且都由「对旧步骤体会失败」的测试钉住。
测试平台
单元套件在 macOS 上运行；异属主复现在 Debian 容器中完成。这些步骤只在 Linux 自建池上运行。
环境（可选）
macOS 上的 Node.js 22 开发检出，Linux 夹具使用 Docker。
actionlint与.github/scripts/check-workflow-size.sh均通过。风险与范围
_qwen-quarantine下累积，因为按定义这个 job 就是删不掉它们。这正是目的——它们离开了 checkout 路径——但该目录确实需要偶尔人工清理，警告中已说明。它只会在本来就会崩掉的 runner 上被写入。remove_review_tree阶梯同样止于「警告后放行」，可以采用相同兜底，这属于后续工作。本 PR 修的是通用路径——每个共享池 job 都会执行的 checkout 前清理——因此任何工作流留下的残留都不再破坏 checkout。它并不阻止残留被产生。关联 Issue
无。观察自 run 32621267802（#9776 与
feat/review-coverage-ledger均在 runner…-18上失败于 Checkout）。