fix(ci): recover protected qwen leftovers before checkout - #10214
Merged
Conversation
deleteTeamDirs() used Promise.allSettled() but ignored rejected results, causing team_delete to report success even when fs.rm failed with non-ENOENT errors (EACCES, EIO, etc.), leaving orphaned directories on disk. Check allSettled results and throw on real failures; ENOENT remains silently ignored for idempotent deletes. Fixes #10210 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
requested review from
LaZzyMan,
doudouOUC,
qqqys,
tanzhenxin and
wenshao
as code owners
August 26, 2026 22:47
Collaborator
Collaborator
Author
|
Correction (not a duplicate): #10213 carries the issue #10210 code fix ( |
- team-delete.ts: catch deleteTeamDirs errors so the state-reset tail (disposeInboxLocks, setTeamManager(null), etc.) always runs even on EACCES/EIO. Logged via debug.warn. - teamHelpers.ts: collect all rejection errors from allSettled; throw AggregateError when both fail, single error when only one fails. Remove unreachable ENOENT special case (force:true already handles it). - teamHelpers.test.ts: update tests for AggregateError behavior; add discriminating test where first rm succeeds and second fails. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> # Conflicts: # packages/core/src/agents/team/teamHelpers.test.ts
Extend the shared-runner pre-checkout sweep to remove both .qwen and the observed .qwen.root-orig recovery backup. Preserve the sudo-less same-filesystem quarantine fallback so abrupt verify termination cannot poison later checkouts. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
changed the title
qwen-code-ci-bot
requested changes
Aug 28, 2026
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.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the scripts suite was not run on a Windows lane locally.
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.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the scripts suite was not run on a Windows lane locally。
未检查（工具限制，非阻断）：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.2)
The sweep rewrite into a for-loop moved the symlink guard from `[ ! -L "$GITHUB_WORKSPACE/.qwen" ]` to the loop variable, leaving the helper test's literal pin red on the required Linux gate. Pin the new chmod-guard shape `[ -d "$stale_qwen" ] && [ ! -L "$stale_qwen" ]` so a guard dropped from the chmod rung fails here; the simpler `[ ! -L ... ]` match alone would survive that mutant via the existence-guard copy. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Every behavioral fixture created plain directories, so dropping `&& [ ! -L "$stale_qwen" ]` from the existence guard left the suite green while a dangling-symlink .qwen hit `continue` and survived the sweep. Add a dangling-symlink leftover to the ci sweep fixture and assert via lstat that it is unlinked, mirroring the existing remove_review_tree symlink coverage; gated on permissionFixturesAvailable so Windows lanes skip instead of erroring on symlinkSync. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This was referenced Aug 28, 2026
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
requested changes
Aug 28, 2026
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.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the scripts suite was not run on a Windows lane locally.
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.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
.github/workflows/ci.yml:224 — [probe] quarantine fallback for .qwen.root-orig and ${stale_name#\.} entry naming have no behavioural test (mutant-proven: hardcoded-prefix mutant keeps suite 17/17 green, nests second tree)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the scripts suite was not run on a Windows lane locally。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
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: 232 passed · 0 failed · 232 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：232 通过 · 0 失败 · 232 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 10214 deep verification —
|
| Cell | Build | Fixture | Observable oracle | Base | Head (ci.yml) | Head (review step) |
|---|---|---|---|---|---|---|
| A | natural ownership | .qwen + .qwen.root-orig dirs + .qwenignore |
leftover entries that would trip checkout | 1 (.qwen.root-orig left behind — the bug) |
0 | 0 |
| B | foreign ownership simulated (rm/chmod PATH-shimmed to EACCES; sudo absent in this container, as on the lane) |
same | workspace clean; both names quarantined under _qwen-quarantine/ with distinct qwen-* / qwen.root-orig-* names; warning per name |
.qwen quarantined, .qwen.root-orig still poisons the workspace |
both quarantined, content intact | both quarantined |
| C | symlink leftovers | dangling and live symlinks for both names | link unlinked; live target untouched and still mode 0444 | n/a (name absent from base loop) | 4/4 clean | — |
| D | read-only self-owned tree | .qwen at 0555/0444 |
chmod-then-rm deletes it | — | clean | — |
| E | negative controls | empty ws / deletable ws | no _qwen-quarantine created when rm succeeds |
— | clean | — |
| F | repeated forced-quarantine runs | two runs | two collision-free quarantine entries | — | clean | — |
Witness: evidence/01-ab-base-vs-head-sweep.png (the full 61-assertion run as printed).
The X/Y flip is exactly the incident's shape: base leaves one tripping entry in the workspace after the sweep (and leaves it inside the workspace even on the quarantine fallback path); head leaves zero on every path. Instrument probe G1 additionally established that on this lane's GNU coreutils, chmod -R u+w on a symlinked argument follows the link — so the PR's [ -d ] && [ ! -L ] chmod guard is load-bearing against real behavior, not just defence-in-depth, and cell C's mode assertion proves the guard holds it.
Structural coverage of the declared jobs
Scripted extraction (part of the 61): 3 byte-identical copies in ci.yml (test, web_shell_e2e_smoke, integration_cli), 1 new step in qwen-code-pr-review.yml (review-pr), none at base in the review workflow. Step-order assertions (restore → sweep → checkout) are pinned by the PR's own suite (19/19, see gates). The review-lane step is the identical loop minus ci.yml's explanatory comments and worktree-prune tail (comment-stripped prefix equality asserted).
Corrections
C1 — "Prettier … passed" (PR body, Reviewer Test Plan). Repo-wide prettier --check (pinned config, v3.6.1 from the lockfile) flags .github/scripts/qwen-triage-workflow.test.mjs at lines 1045–1060 — the flakiness-gate record pins, which this PR does not touch. Measured byte-identical on base and head (both fail the check the same way; the PR's own hunks at lines 682–686 format cleanly). So the claim holds for the changed hunks but not repo-wide; the drift predates this PR. (First measurement attempt was itself vacuous — probe copies under tmp/ are silently skipped because .gitignore lists tmp/ and prettier honors it; re-measured with --ignore-path /dev/null.) Label: correction to the description, not a request to change this PR.
Findings
F1 (Suggestion, pre-existing, unchanged by this PR) — sibling checkout lanes on the same pool remain exposed to the same residue. The residue producer is the verify lane (chown -R root:root .qwen; chmod -R a-w,a+rX .qwen, qwen-triage.yml ≈ line 4060). After a killed verify, the same root-owned tree can trip the next checkout of any job scheduled on that runner. This PR sweeps the four declared jobs; the following ecs-qwen checkouts still have no .qwen/.qwen.root-orig removal before actions/checkout:
qwen-triage.yml :: triage— pre-checkout steps are ownership-restore + agent-state sanitization only;qwen-triage.yml :: tmux-testing— pre-checkout step only handles.qwen/tmp/review-pr-*and a symlinked.qwen;qwen-triage.yml :: verifyitself, internal-PR lane — its "Wipe workspace before external code" is gated toverify_trust == 'external', so internal runs checkout unswept;- the long tail of smaller ecs-qwen workflows that checkout (stale, e2e, serve-ab, autofix lanes, …).
Reproduce the exposure shape: any cell-B fixture where the sweep is absent leaves the tripping entry (that is cell A1/B1's base result). This is the incident's bug class one job over; severity is held to Suggestion because the state is identical at base (the PR worsens nothing and fixes the observed lanes) and the PR's What this changes section scopes itself to exactly the four jobs it touches. A follow-up could move the loop into one shared pre-checkout step for every pool checkout.
F2 (informational) — mutation survivor classified: deleting the entire existence guard is behaviorally a no-op. Matrix row M3b survived both suites: without if [ ! -e … ] && [ ! -L … ]; then continue; fi, absent entries fall through to rm -rf on a nonexistent path (silent success) and dangling links are caught downstream anyway. The guard's only observable content is its -L clause — proven load-bearing by the fine mutant M3a (drop just && [ ! -L "$stale_qwen" ] → the new dangling-symlink test kills it). Classification: redundant defence / clarity guard, correct as it stands; nothing to pin further. Completeness reporting, not a merge condition.
Gates (targeted, at head)
| Gate | Result | Liveness proof |
|---|---|---|
vitest … review-worktree-cleanup-workflow.test.js |
19/19 pass (PR claim reproduced) | M0/M1/M3a/M4/M5 mutants turn it red (witness 02-mutation-matrix.png) |
node --test .github/scripts/qwen-triage-workflow.test.mjs |
118/118 pass | M2 mutant turns it red |
node --test .github/scripts/ci-runner-routing.test.mjs |
21/21 pass (PR claim reproduced) | — |
bash -n on both extracted sweep scripts |
clean | — |
| actionlint (pinned 1.7.12, with shellcheck integration, on the two changed workflows) | clean | schema plant → exit 1; shell plant (with shellcheck on PATH) → exit 1 |
| shellcheck (pinned 0.11.0) on extracted sweeps | clean (repo-wide run: exit 0, pre-existing warnings only in unrelated scripts/test-rewind-e2e.sh) |
planted syntax error → exit 1 |
| ESLint on the two changed test files | clean | repo config |
| Prettier | see C1 | gate-liveness note: tmp/ copies are skipped by default ignore paths |
Mutation matrix (vacuity of the new/changed tests)
Unmutated control green on both suites (vitest 19/19, node:test 118/118) before any kill is credited. Witness: evidence/02-mutation-matrix.png.
| Mutant | vitest | node:test (mjs) | Verdict |
|---|---|---|---|
M0 control: rename _qwen-quarantine |
17/19 — structural pin + behavioral quarantine test fail | 118 | killed (positive control) |
M1: drop .qwen.root-orig from ci.yml loop |
17/19 — pin + removes both known qwen state names fail | 118 | killed |
M2: drop [ ! -L ] from chmod guard |
19/19 green (asserts content, not mode) | 117/118 — repinned guard-shape assertion fails | killed, by the mjs pin exactly as commit 1d639c7 intends |
M3a: drop only -L clause from existence guard |
18/19 — dangling-symlink … -L existence arm fails | 118 | killed by the test commit 9204b17 added |
| M3b: delete entire existence guard | 19/19 | 118 | survived — classified F2 (behaviorally inert deletion) |
| M4: delete the whole new review-workflow step | 18/19 — sweeps both … before the review checkout fails | 118 | killed |
| M5: M1+M4 combined | 16/19 — union of M1 and M4 failures, nothing new | 118 | killed |
No mutant regressed anything unrelated; every kill's failing test is the one its commit claims to pin.
Not covered
- Per-commit attribution: checkout is depth 2 (
git rev-list HEAD^1..HEAD^2lists only the head commit vs 9 in the metadata snapshot) — the aggregateHEAD^1..HEADdiff is what was verified. Note: the snapshot's twoteam_deletecommits (Fixes #10210) contribute nothing net —teamHelpers.tsis byte-identical between base and head (that behavior reached main separately); the branch namefix/issue-10210is a leftover of the branch's origin. - True root-owned fixtures: this container runs as uid 1000 with no sudo/chown capability, so foreign ownership was simulated by PATH-shimming
rm/chmodto deny (documented in the harness). This reproduces the shape of the incident (residue defeats rm+chmod, quarantine rename proceeds), not its cause (an abruptly killed verify leaving root-owned state). Cell G1 and the EACCES-shaped shim errors are the bridge. - Replay calibration: no real emitted artifact to calibrate against (no token; no
previous-report.md); the failing job log of run 33146730771 would have calibrated the base arm's reproduction of the incident. The replay is therefore shape-calibrated only. - yamllint: uninstallable in-container (
pip3/python3-venvdenied, no sudo). actionlint covers the YAML schema layer of both changed workflows. - F1 lanes: enumerated structurally, not behaviorally exercised (their exposure predates the PR).
- Review-lane worktree-prune tail: the new review step intentionally omits ci.yml's post-sweep
git worktree prunetail; inert onceactions/checkoutrecreates.git, but not separately proven. - Windows/macOS and ubuntu-latest lanes: GitHub-hosted or separate pools; no sweep needed by construction.
- No security-sensitive surface changed; the sweep only ever touches the two allowlisted names under
$GITHUB_WORKSPACEand its parent — no untrusted-name globbing was introduced (the narrow allowlist is the right call: globbing runner-resident names on a shared pool would widen an attacker-influenced surface).
Methodology
Environment: the CI verify container itself (node:22-bookworm equivalent; uid 1000, node v22.23.2, no sudo/zstd/shellcheck preinstalled — pinned linters installed via scripts/lint.js --setup), working tree at refs/pull/10214/merge. Sweep scripts were extracted verbatim from git show HEAD^1:… and git show HEAD:… through the yaml parser and executed with bash --noprofile --norc. Behavioral harness: sweep-ab.mjs (fixtures under mkdtemp, per-cell stdout/stderr/exit in raw/); mutation harness: mutation-matrix.mjs operating in a scratch worktree at tmp/mutant-tree (removed after; worktree confirmed clean before removal), suites re-run per mutant with NO_COLOR=1. All assertions are scripted comparisons with encoded expectations (base-cell reds count as passes); totals in assertions.json. Evidence images were produced with scripts/verify-capture.mjs in the same invocation as the measured runs.
Flakiness gate log
rounds=5 files=2 skipped=0
file .github/scripts/qwen-triage-workflow.test.mjs: (cd .) node --test ./.github/scripts/qwen-triage-workflow.test.mjs
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):
.github/scripts/qwen-triage-workflow.test.mjs: PPPPP
scripts/tests/review-worktree-cleanup-workflow.test.js: PPPPP
verdict: pass
summary: 2 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · .github/scripts/qwen-triage-workflow.test.mjs: P (exit 0)
round 1 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
round 2 · .github/scripts/qwen-triage-workflow.test.mjs: P (exit 0)
round 2 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
round 3 · .github/scripts/qwen-triage-workflow.test.mjs: P (exit 0)
round 3 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
round 4 · .github/scripts/qwen-triage-workflow.test.mjs: P (exit 0)
round 4 · scripts/tests/review-worktree-cleanup-workflow.test.js: P (exit 0)
round 5 · .github/scripts/qwen-triage-workflow.test.mjs: 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
Collaborator
|
Gate re-run on the current head ( Template: all the substance is there — what/why (TLDR + incident evidence), reviewer test plan, risk & scope, and the 中文 summary. Headings deviate from the template's names; noted, not blocking. Problem: observed, and independently verified in the first round — the cited job (review-pr, job 98770404551 in run 33146730771) carries GitHub's own annotations: Direction: aligned — this is our own shared ECS pool tripping over its own protection mechanism. No product-surface concerns. Size: no core modules touched — ~123 lines of workflow YAML across Approach: scope still feels right. An explicit two-name allowlist ( Risk: no elevated risk signals (no high-revert paths touched). Moving on to code review. 🔍 中文说明在当前 head（ 模板：实质内容齐全——改动内容/原因（TLDR + 事故证据）、审阅者测试计划、风险与范围、中文说明。小标题与模板命名不一致，仅提示，不阻断。 问题：已观测到，且第一轮已独立核实——所引用的 job（review-pr，run 33146730771 中的 job 98770404551）带有 GitHub 官方 annotation： 方向：对齐——共享 ECS 池被自身保护机制绊倒，不涉及产品面。 规模：未触及核心模块—— 方案：范围依然合理。显式双名白名单（ 风险：无升级风险信号（未触及高回滚率路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Code review — no blockers. Re-walked the final sweep script on this head (four copies: three in
The delta since the last reviewed head ( Test evidence — the gap from the last round is closed. CI on
The central claim — that the sweep recovers both residue names — is substantiated as far as pre-merge CI can take it: the behavioral fixtures execute the real sweep scripts against fixture workspaces, including the quarantine fallback reproduced with a write-denied parent (the same failing syscall as a foreign-owned residue, per the fixture's own rationale). Not verified, and not verifiable until it happens: recovery against genuinely root-owned residue on a live ECS runner — that takes a killed verify job with root privileges, and the next one should show the quarantine warning instead of a dead checkout. No sandboxed-lane line here: this is CI-workflow recovery, not product behavior, and 2b already ran the exact tests that pin the change. Real-scenario testing: N/A — CI infrastructure with no user-visible product surface. 中文说明代码审查——未发现阻断问题。 对当前 head 上的最终清理脚本（共四份拷贝：
自上次审查的 head（ 测试证据——上一轮的缺口已闭合。 核心主张——清理能恢复两种残留名——在合并前 CI 能做到的范围内已被证实：行为 fixture 用真实清理脚本对 fixture 工作区执行，包括用写拒绝的父目录复现 quarantine 兜底（按 fixture 自己的说明，与外部所有残留的失败系统调用相同）。未验证、且在事前无法验证的：在真实 ECS runner 上对真正 root 所有残留的恢复——那需要一个被终止且带 root 权限的 verify job；下一次发生时应当看到 quarantine 警告而不是 checkout 失败。此处不列沙箱验证触发命令：这是工作流恢复而非产品行为，且 2b 已经运行了钉住该变更的测试。 真实场景测试：N/A——CI 基础设施，无用户可见的产品面。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — the single reservation from the last round (no green run on the head) is now settled by real CI evidence; what remains are documented follow-ups, not defects. Stepping back: this is still the fix I would have proposed — extend the existing sweep to the two known state names instead of a broader glob, and put it on the job that actually died. The incident evidence was real and independently verified, the implementation is careful exactly where it needs to be (symlink dereference, permission repair, rename-not-copy), and the test growth across these rounds has been genuine mutation-minded hardening, not coverage padding — both sweep copies now execute in every behavioral fixture, and the YAML delta since the last review is provenance comments only. What keeps this at 4 rather than 5 is scope residue, not doubt about the code: Approving, pinned to the reviewed commit. ✅ 中文说明置信度：4/5 —— 上一轮唯一的保留（该 head 上没有绿色 CI）已由真实 CI 证据解决；剩下的是已记录的后续工作，不是缺陷。 退一步看：这仍然是我会提出的方案——把现有清理扩展到两个已知状态名，而不是更宽的通配，并把它放在实际出事的 job 上。事故证据真实且经独立核实；实现在最需要小心的地方（符号链接解引用、权限修复、改名而非拷贝）处理得当；这几轮的测试增长是真正的面向变异的加固，而不是覆盖率填充——每个行为 fixture 现在都执行两份清理拷贝，且自上次审查以来 YAML 的增量只有来源注释。 给 4 而不是 5 的原因是范围上的残留，而非对代码的疑虑： 批准，并钉住所审查的提交。✅ — Qwen Code · qwen3.8-max Reviewed at |
The two quarantine fixtures still executed ciCleanSteps[0].run only, so the review copy's quarantine fallback — the error path this sweep exists for — had no behavioral witness. Loop both quarantine fixtures over executableCleanCopies, assert the happy path leaves no quarantine directory on either copy, and reword the executableCleanCopies header to match. Mutation-probed: dropping `mkdir -p "$quarantine"` from the qwen-code-pr-review.yml copy now fails the moves-residue fixture at id qwen-code-pr-review.yml; the same suite is green without the mutant. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The round-4 comment claimed no fixture executes the review-yml copy, which this round's executableCleanCopies delta falsified: the dangling-symlink fixture now witnesses the existence guard's -L arm behaviorally on both copies (mutation-probed: dropping the arm fails the fixture at id qwen-code-pr-review.yml once the pin is relaxed). Reword to the true state — only the chmod guard's ! -L arm has no behavioral witness, so its textual pin stays load-bearing. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
requested changes
Aug 28, 2026
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.
Not reviewed: build-and-test — Test (windows-latest / macos-latest, Node 22.x) was skipped in CI and the scripts suite was not run on those lanes locally.
Not explored to full depth (tool budget reached): "agent 3a": none — all planned checks completed (test execution was substituted with direct pin verification against the committed YAML because node_modules is not instal….
Convergence: round 6 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: scripts/tests/review-worktree-cleanup-workflow.test.js (findings in rounds 4, 5; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest / macos-latest, Node 22.x) was skipped in CI and the scripts suite was not run on those lanes locally。
未探索到全部深度（达到工具调用预算）："agent 3a"：none — all planned checks completed (test execution was substituted with direct pin verification against the committed YAML because node_modules is not instal…。
收敛情况：第 6 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：scripts/tests/review-worktree-cleanup-workflow.test.js（第 4、5 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
The 'removes both known qwen state names' fixture is the first in this suite to execute bash without a capability gate: every rung of the sweep ladder is individually guarded (|| true, stderr swallowed), so on a Git-Bash-only PATH lacking coreutils the sweep exits 0 while leaving .qwen on disk and the removal assertion fails. Probe rm/chmod/mv through bash, matching the file's existing bashAvailable/awkAvailable pattern, and skipIf the fixture where the tools are absent; the pure-YAML pins stay ungated. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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.
Not reviewed: build-and-test — Test (windows-latest / macos-latest, Node 22.x) was skipped in CI and the scripts suite was not run on those lanes locally.
Not explored to full depth (tool budget reached): "agent 1a": none — but note I could not run the vitest suite itself (worktree has no node_modules ); the shell probe above substitutes for the behavioral fixtures on this ….
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
scripts/tests/review-worktree-cleanup-workflow.test.js:373 — [review] sweep spawn invocation hand-inlined four timesscripts/tests/review-worktree-cleanup-workflow.test.js:455 — [probe] live-symlink fixture lacks a mode-preservation witness for the chmod ! -L guardscripts/tests/review-worktree-cleanup-workflow.test.js:397 — [probe] ordering pin silently degenerates to > -1 if the restore step is renamed
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest / macos-latest, Node 22.x) was skipped in CI and the scripts suite was not run on those lanes locally。
未探索到全部深度（达到工具调用预算）："agent 1a"：none — but note I could not run the vitest suite itself (worktree has no node_modules ); the shell probe above substitutes for the behavioral fixtures on this …。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ❌ not passed — blocked (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: 94 passed · 1 failed · 95 total Flakiness gate: 中文 — 判定：❌ 不通过 · 阻塞（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：94 通过 · 1 失败 · 95 总计 抖动门： Verification reportPR 10214 deep verification (round 2) —
|
| # | Finding (round 1) | Severity | Status at new head 6aba60f2 |
|---|---|---|---|
| F1 | Sibling checkout lanes on the ecs-qwen pool remain exposed to the same residue | Suggestion | Stands — re-measured at the new head: census over every workflow finds 16 shared-pool checkout jobs, 4 covered by the both-names sweep (the PR's declared jobs), 12 exposed. The list now also contains ci.yml :: integration_no_ak, which escalated into blocker B1 below; the rest (qwen-triage triage / tmux-testing / verify, e2e, npm-cache, qwen-autofix ×3, sdk-java ×2, serve-ab) is unchanged and still Suggestion-grade. |
| F2 | Mutation survivor M3b (deleting the entire existence guard) classified as behaviorally inert | informational | Resolved — commit 10cc3c74 folded the existence-guard shape [ ! -e "$stale_qwen" ] && [ ! -L "$stale_qwen" ] into expectQuarantineFallback; the same mutant now fails the vitest suite (`1 failed |
| C1 | Repo-wide prettier drift in qwen-triage-workflow.test.mjs (body claims "Prettier … passed") |
correction | Stands — prettier 3.6.1 still flags that file (regions 1045–1060 and later-file blocks); all drift sits outside this PR's single hunk (lines 682–686, which format cleanly), and the file is otherwise byte-identical between base and head. The PR-owned files (review-worktree-cleanup-workflow.test.js, both changed workflows) are prettier-clean at head. |
Central claim — re-measured at the new head
A killed /verify run can leave protected project state under either name — .qwen or the recovery backup .qwen.root-orig — and the pre-checkout sweep must recover both or actions/checkout trips with EACCES (incident run 33146730771). The A/B re-extracts the sweep run: blocks verbatim (yaml parser) at HEAD^1/HEAD and executes them under the step's own shell contract (sweep-ab.mjs, 63 assertions):
| Cell | Fixture / condition | Base (ci.yml) | Head ci.yml + review copy | Head integration_no_ak (stale) |
|---|---|---|---|---|
| A | both names + .qwenignore, natural ownership |
1 tripping entry left (.qwen.root-orig) |
0 on both copies, .qwenignore kept, no quarantine |
1 left (B1) |
| B | PATH-shimmed rm/chmod deny (foreign-ownership shape), sudo absent |
.qwen quarantined, .qwen.root-orig still poisons workspace |
both names quarantined, distinct qwen-* / qwen.root-orig-* entries, content intact, one warning per name |
.qwen quarantined, .qwen.root-orig left (B1) |
| C | dangling + live symlinks, both names | skips both (guard requires -d && ! -L) |
links unlinked on both copies; outside target untouched | n/a |
| D | read-only self-owned .qwen |
chmod-then-rm deletes (parity) | same, no quarantine | — |
| E | empty / deletable workspace | — | no quarantine, no warning (fallback stays a fallback) | — |
| F | two consecutive forced-quarantine runs | — | 4 collision-free quarantine entries | — |
| G1 | instrument probe: chmod -R u+w on a symlinked argument |
— | follows the link on this lane's coreutils — the [ ! -L ] chmod guard is load-bearing, not decoration |
— |
| H | stale copy, incident shapes | — | — | exit 0, .qwen handled, .qwen.root-orig survives both paths |
The X/Y flip of the incident shape is intact at the new head: base leaves one tripping entry in every path; the PR's copies leave zero in every path. Structural cells additionally established: 4 sweep copies in ci.yml at both base and head; base copies all old-shape and byte-identical; head copies test / web_shell_e2e_smoke / integration_cli updated and byte-identical; integration_no_ak byte-identical to the base sweep (the merge brought it through untouched); the review-workflow copy's loop is byte-identical to the ci.yml loop modulo comments; no .qwen-prefixed glob in either copy (.qwenignore can never match). Witness: evidence/01-ab-base-vs-head-sweep.png (the full run as printed; the single FAIL line is the merge-head invariant, finding B1).
Findings
B1 (blocker) — the merge head leaves integration_no_ak with the pre-PR sweep; the PR's own suite is red on it
The merge ref was built against a newer main tip (5ae363e2f9) than the PR's last merge of main; that newer main added ci.yml :: integration_no_ak, a checkout job on the same pool whose Clean stale .qwen before checkout step is byte-identical to the base sweep (asserted in the A/B). Consequences, both measured:
- The PR's own gate fails on the merge head, deterministically. Reproduce (PR test-plan command, run at the merge head):
→
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/review-worktree-cleanup-workflow.test.js
Tests 1 failed | 18 passed (19), failing testkeeps every shared-pool ci.yml checkout sweep pinned to paths.tsatexpectQuarantineFallback(missingfor stale_qwen in … .qwen.root-orig). Identical across 3/3 rounds — not flake (witness04-merge-head-suite-red.png). The suite's pool+checkout enumeration (/ubuntu_runner|ecs-qwen/+actions/checkout) correctly picks the job up:integration_no_ak'sruns-onis the sharedubuntu_runneroutput, andclassify_pr :: pick_runnersets that output to["self-hosted", "linux", "x64", "ecs-qwen"]on the routine internal paths (same-repo PRs, trusted authors,merge_group). - The exposure is behavioral, not just a pin violation.
sweep-ab.mjscell H executes the stale copy against incident-shape fixtures: natural ownership leaves.qwen.root-origin the workspace; under the foreign-ownership shim it quarantines.qwenbut still leaves.qwen.root-orig— exactly the incident's poisoning shape, one more lane over.
Blast radius: every integration_no_ak checkout scheduled after a killed verify on the same runner (the residue producer is the verify lane's chown -R root:root .qwen, unchanged by this PR).
Candidate fix (measured) — extend the sweep to the fourth copy
Splice the web_shell_e2e_smoke step block (comment + name + run — the same shape the PR applied to the other three copies) into the integration_no_ak slot. The PR's step comment already anticipates exactly this: "If that producer's naming changes or a third residue name appears, update the for-loop list in every sweep copy." Full diff: raw/candidate-fix.diff (51 lines), excerpt:
# Same pre-checkout recovery as the test job: this job lands on the
# same reused pool, so leftover review worktrees and branches from an
- # interrupted review would break this checkout too.
+ # interrupted review would break this checkout too. The
+ # `.qwen.root-orig` name's provenance (an external recovery tool) is
+ # documented on the test job's copy.
- name: 'Clean stale .qwen before checkout'
run: |-
set -uo pipefail
- if [ -d "$GITHUB_WORKSPACE/.qwen" ] && [ ! -L "$GITHUB_WORKSPACE/.qwen" ]; then
- chmod -R u+w "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || true
+ for stale_qwen in "$GITHUB_WORKSPACE/.qwen" "$GITHUB_WORKSPACE/.qwen.root-orig"; do
+ if [ ! -e "$stale_qwen" ] && [ ! -L "$stale_qwen" ]; then
+ continue
+ fi
+ if [ -d "$stale_qwen" ] && [ ! -L "$stale_qwen" ]; then
+ chmod -R u+w "$stale_qwen" 2>/dev/null || true
+ fiMeasured in a scratch worktree (all three results required of a suggested fix):
- Hostile fixtures come clean: with the fix applied, cell-H shapes flip — the copy is byte-identical to the
testcopy whose cells A/B/C already return 0 residue on every path (behavioral equality by byte-equality, asserted by the harness). - Benign state unchanged: the fix touches only the
integration_no_akblock; the other three ci.yml copies, the review copy, and every test file are untouched. - Suite counts: vitest suite green
19 passed (19)× 5 identical rounds on the fixed tree (it was1 failed | 18 passed× 3 on the merge head); mjs suite stays118/118; the mutation matrix control was run on this same fixed tree.
F1 (Suggestion, carried over, re-measured) — sibling lanes on the same pool remain exposed
Census at the new head (f1-census.mjs): 16 shared-pool checkout jobs; covered by the both-names sweep: ci.yml :: test, ci.yml :: web_shell_e2e_smoke, ci.yml :: integration_cli, qwen-code-pr-review.yml :: review-pr. Still exposed (12): ci.yml :: integration_no_ak (now B1), qwen-triage.yml :: triage / tmux-testing / verify, e2e.yml :: e2e-test-linux, npm-cache.yml :: save (hard-coded ecs-qwen label), qwen-autofix.yml :: issue-autofix / build-cli / review-address, sdk-java.yml :: test / daemon-e2e, serve-ab.yml :: ab — each verified to actually resolve onto the ecs-qwen pool via its runs-on expression. (Round 1 also named "stale": stale.yml runs on the pool but has no actions/checkout step, so it is outside this exposure class; npm-cache.yml/sdk-java.yml were in round 1's "long tail" and are itemized here.) Severity stays Suggestion: the state predates the PR, the PR worsens nothing, and the description scopes itself to the four declared jobs. A follow-up moving the loop into one shared pre-checkout step for every pool checkout would close the class.
Delta verification (commits since round 1)
The six commits added since the round-1 head (5a65f646) are all test/comment-side; each claim was re-driven at the new head:
| Commit | Claim | Measured result |
|---|---|---|
10cc3c74 |
both guard arms pinned on every sweep copy via expectQuarantineFallback |
Mutants dropping either arm from the review copy both fail the suite: existence arm (M-REV-ARM1) killed behaviorally (dangling-symlink fixture, id qwen-code-pr-review.yml) and by the pin; chmod arm (M-REV-ARM2) killed by the pin alone — the live-symlink fixture passes under it (asserts content, not mode), exactly the "no behavioral witness" the next commit documents. The commit's own 1 failed | 7 passed | 11 skipped numbers are a root-lane environment (11 capability skips); this container runs all 19, so per-mutant counts are 1–2 failed here — same kills, different skip ceiling. |
c2750e9f |
behavioral fixtures execute both executable copies | Confirmed by attribution: mutants that break only the review copy now fail fixtures carrying id qwen-code-pr-review.yml (they could not before this commit). |
c056c9d1 |
quarantine fixtures loop over both copies | Mutant dropping mkdir -p "$quarantine" from the review copy fails exactly the pre-checkout sweep moves residue it cannot delete out of the workspace with assertion message qwen-code-pr-review.yml: expected true to be false — the claim's exact shape. Happy-path fixture asserts no quarantine directory on either copy. |
86b0ddfb |
reworded comment: only the chmod arm lacks a behavioral witness | Matches the M-REV-ARM1/ARM2 kill attribution above. |
d136e281 |
comment-only provenance doc for .qwen.root-orig |
Verified comment-only: comment-stripped loop text is byte-identical before/after (A/B structural cell); no producer exists in-tree (grep of the tree matches only the sweep copies and their pins — same as round 1). |
6aba60f2 |
gate the both-names fixture on rm/chmod/mv probed through bash |
Restricted-PATH probe (capability-probe.sh, witness 03-capability-gate-probe.png): with a PATH containing only bash, the gated fixture goes skipped (19 skipped, exit 0) where round-1 logic would have failed it; with the normal PATH the same filter gives 1 passed; an ungated pure-YAML pin (sweeps both known qwen state names before the review checkout) still runs and passes under the bare PATH. Gate is shaped like the file's existing bashAvailable/awkAvailable probes. The other ungated behavioral fixtures remain gated by permissionFixturesAvailable (non-Windows, non-root), i.e. lanes where coreutils exists — the gate's scoping is proportionate. |
Mutation matrix (round 2, on the candidate-fixed tree; control green)
Run on the fixed tree because the merge head's suite is red (B1) — a red control would make kill attribution ambiguous; none of the mutants touches the fixed block. Control: vitest 19/19 + mjs 118/118 green before any kill is credited. Witness evidence/02-mutation-matrix.png; per-mutant logs in raw/mutant-*.log.
| Mutant | vitest | mjs | Verdict |
|---|---|---|---|
M0 control: rename _qwen-quarantine (ci.yml) |
2 failed | 17 passed | — | killed (positive control) |
M1: drop .qwen.root-orig from ci.yml loop |
2 failed | 17 passed | — | killed |
M2: drop ! -L from chmod guard (ci.yml) |
1 failed | 18 passed (new: the vitest pin now catches it too) | 117/118 | killed by both suites |
M3a: drop -L clause from existence guard (ci.yml) |
2 failed | 17 passed | — | killed by the dangling-symlink fixture + pin |
| M3b: delete ENTIRE existence guard (ci.yml) | 1 failed | 18 passed (pin) | — | killed — the round-1 survivor is gone; behavioral fixtures still pass under it (rm on a dangling link silently succeeds), so the pin is what pins it |
| M-REV-ARM1: review copy, existence arm | 2 failed | 17 passed | — | killed (fixture + pin), per 10cc3c74 |
| M-REV-ARM2: review copy, chmod arm | 1 failed | 18 passed (pin only) | — | killed, per 10cc3c74/86b0ddfb |
M-REV-QUAR: review copy, drop mkdir -p "$quarantine" |
1 failed | 18 passed (…moves residue…, id qwen-code-pr-review.yml) |
— | killed, per c056c9d1 |
| M4: delete the whole review-workflow sweep step | 6 failed | 13 passed | — | killed |
(— = the mjs suite was run for the control and M2 only; the vitest suite alone pins every other mutant.)
No mutant regressed from killed to survived versus round 1; every kill's failing test is the one its commit claims to pin.
Gates (targeted, at the merge head unless noted)
| Gate | Result | Liveness proof |
|---|---|---|
vitest review-worktree-cleanup-workflow.test.js |
1 failed | 18 passed (19) — B1 | suite catches all 9 mutants + the unfixed merge head (matrix) |
node --test .github/scripts/qwen-triage-workflow.test.mjs |
118/118 ×5 identical rounds | M2 turns it red (117/118) |
node --test .github/scripts/ci-runner-routing.test.mjs |
21/21 (PR test-plan claim reproduced) | — |
| vitest on candidate-fixed tree | 19/19 ×5 identical rounds | B1's own red is the counter-example |
bash -n on all 4 distinct extracted sweep scripts (base + head) |
clean | — |
shellcheck 0.11.0 -s bash on extracted sweeps; repo-wide run |
clean; repo run exit 0 (pre-existing warnings only in unrelated scripts/test-rewind-e2e.sh) |
planted unclosed [ → exit 1 |
| actionlint 1.7.12 on the two changed workflows; repo-wide | clean | planted bogus_key: on a step → unexpected key exit 1 |
| ESLint on both changed test files | clean | planted unused variable → error exit 1 |
| prettier 3.6.1 | see C1 (PR-owned files clean) | — |
| Flakiness | mjs PPPPP; vitest merge head F F F byte-identical summaries (deterministic red, not flake); fixed tree PPPPP |
speed-correlation n/a: failure is content-driven |
Not covered
- Per-commit attribution: depth-2 checkout (
git rev-list HEAD^1..HEAD^2= 1 commit vs 15 in the metadata snapshot); the aggregateHEAD^1..HEADdiff is what was verified. As in round 1, the twoteam_deletecommits in the snapshot contribute nothing net (no package files in the aggregate diff). - True root-owned fixtures: uid 1000 container, no sudo/chown — foreign ownership simulated via PATH-shimmed
rm/chmod(documented in the harness). Reproduces the incident's shape (residue defeats rm+chmod; quarantine rename proceeds), not its cause (a killed verify leaving root-owned state). Cell G1 plus the EACCES-shaped shim errors are the bridge, same as round 1. - Replay calibration: no real emitted artifact to calibrate against (no token; the failing job log of run 33146730771 is unreachable) — replays remain shape-calibrated only.
- yamllint: uninstallable in-container (
pip3permission-denied). actionlint covers the YAML schema layer of both changed workflows. - F1 lanes: enumerated structurally (runs-on expressions traced to the ecs pool), not behaviorally exercised — their exposure predates the PR.
- Snapshot base drift:
$QWEN_VERIFY_CONTEXTcarriesbaseRefOid d6533785…while the merge ref's actual base parent is5ae363e2f9…(main moved between snapshot and merge-ref build); per the environment contractHEAD^1was used as the base throughout. The consequence of that drift —integration_no_ak— is exactly finding B1. - Windows/macOS and ubuntu-latest lanes: GitHub-hosted or separate pools; no sweep needed by construction.
- No security-sensitive surface changed; the sweep touches only the two allowlisted names under
$GITHUB_WORKSPACE; no globbing of runner-resident names was introduced.
Methodology
Environment: the CI verify container (uid 1000, node v22.23.2, no sudo/zstd/shellcheck preinstalled — pinned linters installed via scripts/lint.js --setup; yamllint refused), working tree at refs/pull/10214/merge. Sweep scripts were extracted verbatim from git show HEAD^1:… / git show HEAD:… through the yaml parser and executed with bash --noprofile --norc. Behavioral harness sweep-ab.mjs (fixtures under mkdtemp; per-cell stdout/stderr/exit in raw/cells.log); mutation harness mutation-matrix.mjs in a scratch worktree at tmp/mutant-tree (removed after, confirmed clean); candidate fix applied by apply-fix.mjs (step-block splice, byte-equality asserted post-splice); capability probe capability-probe.sh; census f1-census.mjs. All assertions are scripted comparisons with encoded expectations (base-cell reds and the deterministic merge-head red count as passed expectations only where the expectation is "this arm must be red"; the one unexpected outcome — the merge-head invariant — counts as the single fail). Evidence images were produced with scripts/verify-capture.mjs in the same invocation as the measured runs.
Flakiness gate log
rounds=5 files=2 skipped=0
file .github/scripts/qwen-triage-workflow.test.mjs: (cd .) node --test ./.github/scripts/qwen-triage-workflow.test.mjs
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):
.github/scripts/qwen-triage-workflow.test.mjs: PPPPP
scripts/tests/review-worktree-cleanup-workflow.test.js: FFFFF
verdict: consistent-fail
summary: 1 of 2 changed test file(s) failed identically in every round — deterministic, so CI owns that signal
--- per-invocation detail (full copy in the artifact) ---
round 1 · .github/scripts/qwen-triage-workflow.test.mjs: P (exit 0)
round 1 · scripts/tests/review-worktree-cleanup-workflow.test.js: F (exit 1)
--- output tail · round 1 · scripts/tests/review-worktree-cleanup-workflow.test.js ---
�[1m�[46m RUN �[49m�[22m �[36mv3.2.7 �[39m�[90m/__w/qwen-code/qwen-code�[39m
�[31m❯�[39m scripts/tests/review-worktree-cleanup-workflow.test.js �[2m(�[22m�[2m19 tests�[22m�[2m | �[22m�[31m1 failed�[39m�[2m)�[22m�[32m 199�[2mms�[22m�[39m
�[31m �[31m�[31m review worktree cleanup steps�[2m > �[22mkeeps every shared-pool ci.yml checkout sweep pinned to paths.ts�[39m�[32m 24�[2mms�[22m�[39m
�[31m → expected 'set -uo pipefail\nif [ -d "$GITHUB_WO…' to contain 'for stale_qwen in "$GITHUB_WORKSPACE/…'�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremoves both known qwen state names without touching .qwenignore�[32m 18�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22msweeps both known qwen state names before the review checkout�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mpre-checkout sweeps unlink a dangling-symlink .qwen via the -L existence arm�[32m 11�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mpre-checkout sweeps unlink a live-symlink .qwen without touching its target�[32m 11�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mkeeps the review-job cleanup sweep pinned to paths.ts�[32m 4�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mkeeps the pre-checkout agent-state sweep pinned to paths.ts�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22muses one identical worktree filter at every list-driven sweep�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mfilter selects review worktrees only, never the main checkout�[32m 5�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mthe pre-checkout sweep moves residue it cannot delete out of the workspace�[32m 34�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mthe pre-checkout sweep still deletes residue it can remove�[32m 12�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremove_review_tree actually removes a plain leftover�[32m 7�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremove_review_tree refuses a symlinked leftover without touching its target�[32m 9�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremove_review_tree keeps a newline-bearing leftover name on one warning line�[32m 13�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremove_review_tree unlinks a symlinked leftover over a writable parent�[32m 6�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremove_review_tree removes a dangling symlink via the -L existence arm�[32m 6�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremove_review_tree refuses a path outside the workspace�[32m 11�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremove_review_tree repairs a permission-locked tree and then removes it�[32m 10�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mskip warnings keep a CR-bearing registered path on one runner line�[32m 11�[2mms�[22m�[39m
�[31m⎯⎯⎯⎯⎯⎯⎯�[39m�[1m�[41m Failed Tests 1 �[49m�[22m�[31m⎯⎯⎯⎯⎯⎯⎯�[39m
�[41m�[1m FAIL �[22m�[49m scripts/tests/review-worktree-cleanup-workflow.test.js�[2m > �[22mreview worktree cleanup steps�[2m > �[22mkeeps every shared-pool ci.yml checkout sweep pinned to paths.ts
�[31m�[1mAssertionError�[22m: expected 'set -uo pipefail\nif [ -d "$GITHUB_WO…' to contain 'for stale_qwen in "$GITHUB_WORKSPACE/…'�[39m
�[32m- Expected�[39m
�[31m+ Received�[39m
�[32m- for stale_qwen in "$GITHUB_WORKSPACE/.qwen" "$GITHUB_WORKSPACE/.qwen.root-orig"; do�[39m
�[31m+ set -uo pipefail�[39m
�[31m+ if [ -d "$GITHUB_WORKSPACE/.qwen" ] && [ ! -L "$GITHUB_WORKSPACE/.qwen" ]; then�[39m
�[31m+ chmod -R u+w "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || true�[39m
�[31m+ rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null ||�[39m
�[31m+ sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null ||�[39m
�[31m+ {�[39m
�[31m+ quarantine="$(dirname -- "$GITHUB_WORKSPACE")/_qwen-quarantine"�[39m
�[31m+ mkdir -p "$quarantine" 2>/dev/null || true�[39m
�[31m+ if mv -- "$GITHUB_WORKSPACE/.qwen" "$quarantine/qwen-$(date -u +%Y%m%dT%H%M%SZ)-$$" 2>/dev/null; then�[39m
�[31m+ echo "::warning::could not delete leaked .qwen; moved it to $quarantine so this checkout can proceed — that directory needs manual cleanup"�[39m
�[31m+ else�[39m
�[31m+ echo "::warning::leaked .qwen; runner needs manual cleanup"�[39m
�[31m+ fi�[39m
�[31m+ }�[39m
�[31m+ fi�[39m
�[31m+ if [ -e "$GITHUB_WORKSPACE/.git" ]; then�[39m
�[31m+ GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")�[39m
�[31m+ "${GIT_SAFE[@]}" worktree prune -v || true�[39m
�[31m+ "${GIT_SAFE[@]}" worktree list --porcelain \�[39m
�[31m+ | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \�[39m
�[31m+ | while read -r worktree; do�[39m
�[31m+ [ -n "$worktree" ] || continue�[39m
�[31m+ case "$worktree" in�[39m
�[31m+ */../*|../*|*/..)�[39m
�[31m+ echo "::warning::skipping suspicious review worktree path: $worktree"�[39m
�[31m+ continue�[39m
�[31m+ ;;�[39m
�[31m+ "$GITHUB_WORKSPACE/.qwen/tmp/review-pr-"*) : ;;�[39m
�[31m+ *)�[39m
�[31m+ echo "::warning::skipping unexpected review worktree path: $worktree"�[39m
�[31m+ continue�[39m
�[31m+ ;;�[39m
�[31m+ esac�[39m
�[31m+ "${GIT_SAFE[@]}" worktree remove --force "$worktree" ||�[39m
�[31m+ echo "::warning::could not remove review worktree: $worktree"�[39m
�[31m+ done || true�[39m
�[31m+ "${GIT_SAFE[@]}" worktree prune -v || true�[39m
�[31m+ "${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \�[39m
�[31m+ | while read -r stale_ref; do�[39m
�[31m+ if [ -n "$stale_ref" ]; then�[39m
�[31m+ "${GIT_SAFE[@]}" branch -D "$stale_ref" ||�[39m
�[31m+ echo "::warning::could not remove review branch: $stale_ref"�[39m
�[31m+ fi�[39m
�[31m+ done || true�[39m
�[31m+ fi�[39m
�[36m �[2m❯�[22m expectQuarantineFallback scripts/tests/review-worktree-cleanup-workflow.test.js:�[2m173:16�[22m�[39m
�[90m171| �[39m // the for-loop list in ci.yml and qwen-code-pr-review.yml, or the s…
�[90m172| �[39m �[90m// silently no-ops and the checkout poisoning recurs.�[39m
�[90m173| �[39m �[34mexpect�[39m(code)�[33m.�[39m�[34mtoContain�[39m(
�[90m | �[39m �[31m^�[39m
�[90m174| �[39m 'for stale_qwen in "$GITHUB_WORKSPACE/.qwen" "$GITHUB_WORKSPACE/.q…
�[90m175| �[39m )�[33m;�[39m
�[90m �[2m❯�[22m scripts/tests/review-worktree-cleanup-workflow.test.js:�[2m347:7�[22m�[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯�[22m�[39m
�[2m Test Files �[22m �[1m�[31m1 failed�[39m�[22m�[90m (1)�[39m
�[2m Tests �[22m �[1m�[31m1 failed�[39m�[22m�[2m | �[22m�[1m�[32m18 passed�[39m�[22m�[90m (19)�[39m
�[2m Start at �[22m 02:37:50
�[2m Duration �[22m 6.21s�[2m (transform 3.19s, setup 31ms, collect 5.22s, tests 199ms, environment 0ms, prepare 192ms)�[22m
round 2 · .github/scripts/qwen-triage-workflow.test.mjs: P (exit 0)
round 2 · scripts/tests/review-worktree-cleanup-workflow.test.js: F (exit 1)
--- output tail · round 2 · scripts/tests/review-worktree-cleanup-workflow.test.js ---
�[1m�[46m RUN �[49m�[22m �[36mv3.2.7 �[39m�[90m/__w/qwen-code/qwen-code�[39m
�[31m❯�[39m scripts/tests/review-worktree-cleanup-workflow.test.js �[2m(�[22m�[2m19 tests�[22m�[2m | �[22m�[31m1 failed�[39m�[2m)�[22m�[32m 220�[2mms�[22m�[39m
�[31m �[31m�[31m review worktree cleanup steps�[2m > �[22mkeeps every shared-pool ci.yml checkout sweep pinned to paths.ts�[39m�[32m 30�[2mms�[22m�[39m
�[31m → expected 'set -uo pipefail\nif [ -d "$GITHUB_WO…' to contain 'for stale_qwen in "$GITHUB_WORKSPACE/…'�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mremoves both known qwen state names without touching .qwenignore�[32m 23�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22msweeps both known qwen state names before the review checkout�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mpre-checkout sweeps unlink a dangling-symlink .qwen via the -L existence arm�[32m 11�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mpre-checkout sweeps unlink a live-symlink .qwen without touching its target�[32m 12�[2mms�[22m�[39m
�[32m✓�[39m review worktree cleanup steps�[2m > �[22mkeeps the review-job cleanup sweep pinned to paths.ts�[32m 7�[2mms�[22m�[39m
...truncated -- full content in the run artifacts.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
previously approved these changes
Aug 29, 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. ✅
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 29, 2026
Collaborator
Local verification of #10214 — real fake-runner harness, head
|
| runner | what happens | needs this PR? |
|---|---|---|
| passwordless sudo works | Restore workspace ownership's sudo -n chown -R already makes the residue deletable, and actions/checkout removes it by itself — verified: base + sudo ⇒ checkout OK |
no |
sudo refuses (the incident runner, …-22) |
chmod fails → rm fails → sudo -n rm fails → cross-parent mv fails ⇒ residue survives, checkout dies |
yes, but the PR does not deliver it |
Its own regression fixture passes because it models the poison as a chmod-500 inner directory under a runner-owned top level; there the cross-parent rename is legal. Make the top-level directory foreign-owned — as the external tool that produces .qwen.root-orig does — and the fallback stops working.
A rung that does work, sudo-less, verified end to end: the workspace directory is always the runner's own, so it can always be renamed. Appending this to the sweep turns the same fixture green:
# after the existing per-name ladder
for stale_qwen in "$GITHUB_WORKSPACE/.qwen" "$GITHUB_WORKSPACE/.qwen.root-orig"; do
if [ -e "$stale_qwen" ] || [ -L "$stale_qwen" ]; then
quarantine="$(dirname -- "$GITHUB_WORKSPACE")/_qwen-quarantine"
mkdir -p "$quarantine" 2>/dev/null || true
if mv -- "$GITHUB_WORKSPACE" "$quarantine/workspace-$(date -u +%Y%m%dT%H%M%SZ)-$$" 2>/dev/null &&
mkdir -p "$GITHUB_WORKSPACE"; then
echo "::warning::could not delete leaked qwen state; moved the whole workspace to $quarantine"
else
echo "::warning::leaked qwen state survived every recovery; runner needs manual cleanup"
fi
break
fi
doneCaveats worth weighing if you take it: it discards the whole warm workspace (caches, node_modules), the step's own cwd keeps pointing at the moved inode for the rest of that step, and _qwen-quarantine still has no retention policy. The alternative that removes the class entirely is fixing the producer (a runner-level ACTIONS_RUNNER_HOOK_JOB_STARTED cleanup, or passwordless chown/rm for the runner user) rather than chasing names in every consumer.
2. main has grown a fourth shared-pool checkout job — as-is, this PR red-lights its own gate after merge
main@ad0061442b added ci.yml job integration_no_ak carrying the pre-PR sweep (.qwen only). The PR's gate enumerates every shared-pool checkout job and additionally pins all copies byte-identical, so on the merged tree:
× keeps every shared-pool ci.yml checkout sweep pinned to paths.ts
→ expected 'set -uo pipefail\nif [ -d "$GITHUB_WO…' to contain 'for stale_qwen in "$GITHUB_WORKSPACE/…'
Tests 1 failed | 18 passed (19) (19 passed | 19 on the PR head alone)
Merge main and update the fourth copy. Beyond that, 11 shared-pool checkout jobs still have no sweep at all — pre-existing, not this PR's doing, but the most interesting one is qwen-triage.yml verify, the very job whose container produces the root-owned state.
3. What I confirmed does hold
Run as the unprivileged runner user, under bash -e (how Actions invokes run:), against the real extracted step scripts — 18 assertions pass, the 3 failures are all block [5], the incident shape:
- both names deleted when the runner can delete them;
.qwenignoreuntouched; no quarantine directory and no warning on a clean sweep; - dangling symlink unlinked (the
-Lexistence arm is load-bearing —-ealone reports it absent) and a live symlink unlinked with its outside target and contents intact (the! -Lchmod arm); set -uo pipefail+ the|| trueladder never fails the step (exit 0 in every scenario, including the unrecoverable one);- the review copy behaves identically to the
ci.ymlcopy, and it sits in the right place: afterRestore workspace ownership, beforeCheckout base branch; npx vitest … review-worktree-cleanup-workflow.test.js→ 19/19,node --test .github/scripts/qwen-triage-workflow.test.mjs→ 118/118,ci-runner-routing.test.mjs→ 21/21,git diff --checkclean.
4. Smaller notes
- The suites are root-hostile. As
rootthe same runs give8 passed | 11 skippedand82 pass | 36 fail—permissionFixturesAvailablegates ongeteuid() !== 0, so every behavioural fixture silently disappears in a root container. Worth stating in the test plan; "19/19 passed" is only true off-root. _qwen-quarantineis created even when the move fails, leaving an empty directory on the pool each time.- The allowlist is coupled to an out-of-repo name.
git grep root-origmatches only the sweeps and their pins — nothing in this repo emits.qwen.root-orig, so a rename on the producer's side silently no-ops the sweep with no test able to catch it. The PR says this; I'm confirming it, and it is an argument for a shape-based last resort (item 1) over a name list. prettier --check .github/scripts/qwen-triage-workflow.test.mjswarns — but identically on the merge-base and onmain, so it is pre-existing, not this PR.
Repro
git fetch origin pull/10214/head && git worktree add wt-pr10214 FETCH_HEAD && (cd wt-pr10214 && npm ci)
useradd -m ciuser # the sweep must NOT be run as root
# fixture: workspace owned by ciuser, .qwen.root-orig root-owned + chmod a-w,
# PATH shim making `sudo` print "sudo: a password is required" and exit 1
su ciuser -c 'GITHUB_WORKSPACE=$WS bash -e <step-script-extracted-from-ci.yml>'
su ciuser -c 'GITHUB_WORKSPACE=$WS node checkout-sim.mjs' # fs.rm recursive, as @actions/io rmRFScope of this verification
Everything above ran locally (Linux, Node v22.22.2, real npm ci in a clean worktree at 6aba60f2). I did not run anything on the ECS pool itself, and I did not reproduce the /verify container that creates the residue — I recreated its output shape from the failing job's own log, and modelled the sudo refusal on that log's exact message. Advisory input for the merge decision, not an approval.
中文版（合并参考）
结论
我没有只读 YAML，而是在本地搭了一个"假 ECS runner"环境复现事故：workspace 属于非 root 的 ciuser，里面放一个 root 所有、只读的 .qwen.root-orig，用 PATH shim 让 sudo 输出与事故 runner 完全一致的 sudo: a password is required，执行的是从 PR 的 YAML 里直接抽出来的真实 step 脚本，再用 fs.rm(recursive/force/maxRetries:3)（即 @actions/io 的 rmRF）复刻 actions/checkout 对复用 workspace 的清理。
结论：对"能删掉的残留"这个 PR 写得对、测得也扎实；但在事故实际发生的那台 runner 上它仍然救不回来，checkout 依旧以完全相同的 EACCES 失败。合并前建议解决两点。
一、run 33146730771 的残留仍未被恢复（阻塞该 PR 自述目标）
同一 fixture、同一批 step，只有 sweep 不同（见第 1 张图）。PR 唯一可观测的变化是多了一行 ::warning::leaked .qwen.root-orig; runner needs manual cleanup —— 这是兜底失败分支，说明 quarantine 的 mv 也失败了；随后的 checkout / wipe / retry 与真实 job log 逐行一致，连 workspace wipe left survivors: …/.qwen.root-orig 都一样。
原因（见第 2 张图）：把目录移到另一个父目录时内核要改写它的 ..，因此 rename(2) 需要被移动目录自身的写权限；root 所有的 dr-xr-xr-x 残留恰好不给 → 必然 EACCES（同父目录改名则成功，这是探针里的对照组）。真实日志也印证残留就是这种形状：rm: cannot remove '$GITHUB_WORKSPACE/.qwen.root-orig/review-context.json': Permission denied 是直接子项，说明 .qwen.root-orig 本身对 runner 用户不可写。而 sudo -n rm -rf 这一级在那台机器上是死的：日志里 sudo: a password is required 出现了两次。
于是两条分支互斥，PR 都没落到：
| runner | 结果 | 需要本 PR 吗 |
|---|---|---|
| 有免密 sudo | Restore workspace ownership 的 sudo -n chown -R 已经把残留变成可删，actions/checkout 自己就清掉了（已验证：base + sudo ⇒ checkout OK） |
不需要 |
sudo 拒绝（事故 runner …-22） |
chmod 失败 → rm 失败 → sudo rm 失败 → 跨父目录 mv 失败 ⇒ 残留存活、checkout 死 | 需要，但 PR 没提供 |
PR 自带的回归 fixture 之所以是绿的，是因为它把"毒"建模成runner 自己拥有的顶层目录 + 里面 chmod 500 的子目录，这种形状跨父目录改名是合法的；一旦顶层目录属于别的 uid（外部工具产出 .qwen.root-orig 的真实情况），兜底就失效。
一条 sudo-less 且经端到端验证可行的兜底（第 3 张图）：workspace 目录永远是 runner 自己的，因此永远能被改名 —— 在现有阶梯后追加"整个 workspace 挪走并重建空目录"，同一 fixture 立刻变绿（代码见英文版）。代价：会丢掉整个热 workspace（缓存、node_modules），该 step 剩余部分的 cwd 仍指向被移动的 inode，且 _qwen-quarantine 依然没有清理策略。更彻底的做法是修生产者（runner 级 ACTIONS_RUNNER_HOOK_JOB_STARTED 清理，或给 runner 用户免密 chown/rm），而不是在每个消费者里追名字。
二、main 又多了一个共享池 checkout job —— 现状下合并 main 会让本 PR 自己的门禁变红
main@ad0061442b 新增了 ci.yml 的 integration_no_ak，带的是旧 sweep（只扫 .qwen）。而 PR 的门禁会枚举所有共享池 checkout job 并要求各副本逐字节一致，所以在合并后的树上（第 4 张图）：
× keeps every shared-pool ci.yml checkout sweep pinned to paths.ts
Tests 1 failed | 18 passed (19) （只看 PR head 时是 19/19）
需要 merge main 并同步第 4 份副本。另外还有 11 个共享池 checkout job 完全没有 sweep（属于既有问题，不是本 PR 造成），其中最值得注意的是 qwen-triage.yml 的 verify —— 正是产生 root 残留的那个容器 job。
三、确认成立的部分（第 5 张图）
以非 root 用户、在 bash -e（Actions 真实调用方式）下跑抽取出来的真实 step：18 条断言通过，3 条失败全部集中在第 [5] 组即事故形状。
- 可删时两个名字都被删除；
.qwenignore不受影响；干净场景下不创建 quarantine 目录、不产生 warning； - 悬空 symlink 被 unlink（
-L存在性分支是关键，-e会跟随链接判为不存在）；活 symlink 被 unlink 且外部目标与内容完好（! -L的 chmod 分支）； set -uo pipefail+|| true阶梯在所有场景下都不会让 step 失败（含不可恢复场景，退出码均为 0）；- review 副本与
ci.yml副本行为一致，且位置正确：在Restore workspace ownership之后、Checkout base branch之前； vitest … review-worktree-cleanup-workflow.test.js19/19、qwen-triage-workflow.test.mjs118/118、ci-runner-routing.test.mjs21/21、git diff --check干净。
四、其它小问题
- 测试套件对 root 不友好：以 root 运行同样的命令得到
8 passed | 11 skipped与82 pass | 36 fail（permissionFixturesAvailable用geteuid() !== 0把关）。建议在 Test Plan 里注明 —— "19/19" 只在非 root 下成立。 - 即使 mv 失败也会创建
_qwen-quarantine，每次都会在池上留一个空目录。 - 白名单绑定在仓库外部的名字上：
git grep root-orig只命中 sweep 自身与其断言，仓库内没有任何东西产生.qwen.root-orig；上游一改名，sweep 就静默空转且没有任何测试能发现。PR 自己也写了这一点 —— 这恰恰是"按形状兜底"（第一条）比"按名字列表"更稳的理由。 prettier --check .github/scripts/qwen-triage-workflow.test.mjs会告警，但在 merge-base 与main上同样告警，属既有问题，与本 PR 无关。
验证边界
以上全部在本地完成（Linux、Node v22.22.2、在 6aba60f2 的干净 worktree 中真实 npm ci）。我没有在 ECS 池上跑任何东西，也没有复现产生残留的 /verify 容器本身 —— 我是依据失败 job 自己的日志复现了它的产物形状，并按该日志里的原话建模了 sudo 拒绝。本评论是合并决策的参考证据，不是 approval。
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Addressed the blocking incident replay in The per-state quarantine remains the first fallback. If the foreign-owned state directory cannot cross into that quarantine, the sweep now moves the runner-owned workspace itself aside, recreates an empty The base refresh in Verification:
@wenshao, this implements the workspace-level fallback from your runner harness. The new-head CI has been triggered. |
chiga0
approved these changes
Aug 29, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings. Approve.
What was checked
Shell logic (ci.yml + qwen-code-pr-review.yml — 5 sweep copies):
- Existence guard
[ ! -e "$stale_qwen" ] && [ ! -L "$stale_qwen" ]— correct:-efollows symlinks, so a dangling link satisfies! -ebut! -Lis false, keeping the loop from skipping it. ✓ chmodguard[ -d "$stale_qwen" ] && [ ! -L "$stale_qwen" ]— preventschmod -R u+wfrom dereferencing a live symlink into an outside tree. ✓stale_nameis set as the first line inside the compound{}before any use; noset -uhazard. ✓mv --positional layout:$1=--,$2=source. Test mockif [ "$2" = "$GITHUB_WORKSPACE/.qwen.root-orig" ]matches correctly. ✓breakafter workspace quarantine: recreated workspace is empty; subsequent iterations hitcontinue. ✓${stale_name#\.}strips leading dot for quarantine directory name (.qwen.root-orig→qwen.root-orig). ✓- All 5 copies are structurally identical per the diff; no divergence introduced. ✓
Test coverage (scripts/tests/review-worktree-cleanup-workflow.test.js):
sweepToolsAvailableprobescommand -v rm/chmod/mvbefore executing bash-spawning fixtures — addresses R6-1 (the Windows nightly gate). ✓executableCleanCopiesincludes both ci.yml and qwen-code-pr-review.yml copies; each behavioral test loops over both with independentmkdtempworkspaces — addresses R3-1. ✓- Dangling-symlink fixture witnesses the
-Lexistence arm; live-symlink fixture confirms target untouched — addresses R1-S1 / R2-1. ✓ - Workspace-quarantine fixture (
.qwen.root-origcannot move, workspace itself can): mockedmvreturns 1 when$2 = $GITHUB_WORKSPACE/.qwen.root-orig, passes through for the workspace move. Both copies witness thebreak+cd "$GITHUB_WORKSPACE"recovery path. ✓
Cross-check against bot's open findings:
- R1-1 (qwen-triage assertion hardcoded
.qwen): fixed —qwen-triage-workflow.test.mjsnow asserts$stale_qwen. ✓ - R1-2 (qwen-code-pr-review.yml missing the sweep): fixed — new step added. ✓
- R3-2 (
.qwen.root-origundocumented external origin): addressed — comments in both files document the external tool, the first observed run ID, and the update obligation. ✓ - R6-1 (ungated Windows test): fixed by
sweepToolsAvailable+it.skipIf. ✓ - Bot's round-8 LGTM (dismissed): independently consistent with the above analysis.
CI
Integration Tests (no-AK, No Sandbox)— PASSDesktop Shell— PASSTest (ubuntu-latest, Node 22.x)— PENDING at review time; new JS tests expected to pass (behavioral logic is correct,sweepToolsAvailablegates properly)- macOS / Windows — SKIPPING (pre-existing, not introduced by this PR;
sweepToolsAvailablegate handles the behavioral fixtures)
Reviewed with AI assistance.
Collaborator
Local verification on a real self-hosted-runner environment — no blocking issues foundI rebuilt the incident environment locally instead of re-running the fixtures, because every existing behavioural test models the failure with shell-function shims ( Verdict: the change does what it claims, and it repairs one more thing than the description says. Details below; three non-blocking observations at the end. Environment
1. Incident reproduced, then fixed
The probe at the top is the exact call
2. The mechanism holds at the syscall level
The PR's central claim — "a foreign-owned directory cannot always be renamed to a different parent: updating its This means the PR fixes more than the new name. On The 3. Behaviour matrix (16 scenarios, each in a fresh container)
4. Tests and gates
Mutation-tested to confirm the new assertions are load-bearing:
5. Non-blocking observations(a) The rung that keeps the cache lost its behavioural witness. The suite this PR rewrites used to prove the per-state quarantine behaviourally; the replacement proves the whole-workspace one. So a mutation that silently disables the per-state rung (
That shape (S16 — a runner-owned (b)
An age-gated sweep (like the existing (c) 15 of 20 pool-capable checkout jobs are still unswept.
They share one Also worth knowing: the incident runner evidently has no passwordless sudo (S10 — otherwise the ownership-restore step's Reproducing thisgh pr diff 10214 >/dev/null # PR head 5fc148e7, base 348301c3
# extract each 'Clean stale .qwen before checkout' step's `run:` with js-yaml,
# then in a node:22-bookworm container as a non-root user:
# mkdir -p $WS/.qwen.root-orig/agents && chown -R root:root … && chmod -R 0555 …
# runuser -u runner -- bash --noprofile --norc -eo pipefail <sweep>
# runuser -u runner -- node actions-checkout-v4/dist/index.js
npx vitest run --config ./scripts/tests/vitest.config.ts \
scripts/tests/review-worktree-cleanup-workflow.test.js
node --test .github/scripts/ci-runner-routing.test.mjs
bash .github/scripts/check-workflow-size.sh中文说明本地真实环境验证 —— 未发现阻塞问题我没有直接复跑仓库里的 fixture，而是在本地重建了事故环境：现有行为测试都用 shell 函数桩（ 结论：这个改动确实做到了它声称的事，而且比描述里多修了一处。 三条非阻塞观察见文末。 环境
1. 事故复现，然后被修好（图 1）最上面那条探针就是
2. 机制在 syscall 层面成立（图 2）PR 的核心论断——"foreign-owned 目录不一定能重命名到另一个父目录：即使 workspace 父目录可写，更新它的 这说明 PR 修的不只是新名字。 在
3. 行为矩阵（16 个场景，每个都在全新容器里跑）见英文表格。要点： 4. 测试与门禁
5. 非阻塞观察(a) 保住缓存的那一级失去了行为见证（图 6）。 被本 PR 重写的用例，原来是给单目录 quarantine 做行为证明的，替换后证明的是整体 workspace 那一级。于是"悄悄让单目录那级失效"的变异（ (b) (c) 20 个可能落到 ECS 池且会 checkout 的 job 里，还有 15 个没有这道 sweep（图 5）。 它们在同一个池成员上共用同一个 另外值得知道的：事故 runner 显然没有免密 sudo（S10——否则 ownership restore 里的 Verified locally against PR head |
wenshao
approved these changes
Aug 29, 2026
This was referenced Aug 29, 2026
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
Recovers protected Qwen state left behind by an abruptly terminated containerized verification job before the next shared ECS job reaches checkout.
Every affected pre-checkout sweep now handles the exact allowlist
.qwenand.qwen.root-orig: repair permissions and remove normally, use passwordless sudo when available, then fall back to a same-filesystem quarantine rename. If a foreign-owned state directory cannot cross into that quarantine, the runner-owned workspace is moved aside as the final fallback, an empty checkout root is recreated, and the step changes into it. Similarly named files such as.qwenignoreare not matched.Why it's needed
The existing
if: always()ownership restore covers normal completion and ordinary failures, but cannot run after abrupt worker termination. In the observed incident,.qwen.root-origremained root-owned and read-only, and the next checkout failed withEACCESbecause the old sweep handled only.qwen.Reviewer Test Plan
Verify that the review, test, Web Shell smoke, and integration jobs recover both exact state names before checkout; that
.qwenignoresurvives; and that deletion failures first preserve the warm workspace through per-state quarantine, then quarantine the whole workspace only when the foreign-owned state itself cannot move.How to verify
Run:
Expect both state names to be removed by the fixture while
.qwenignoreremains, and every affected ownership-restore/cleanup step to appear beforeactions/checkout.Evidence (Before & After)
Before: run 33146730771, job 98770404551 failed on runner
ecs-qwen-runner-hk-j6c03lyei7s809zq1s6t-22withEACCES: permission denied, rmdirat.qwen.root-orig/agents.After: the focused workflow cleanup and workflow-size suites passed 191 tests with 23 platform/capability skips. The cleanup suite is 19/19 and includes the exact delete-fails → state-move-fails → workspace-move-succeeds branch; ownership cleanup subtests passed 2/2, and Prettier plus
git diff --checkpassed.Tested on
Risk & Scope
.qwenand.qwen.root-orig._qwen-quarantinefor manual cleanup.Linked Issues
Incident evidence: run 33146730771, job 98770404551.
中文说明
本 PR 做什么
在共享 ECS job checkout 前恢复异常终止的容器验证遗留的受保护 Qwen 状态。所有相关清理步骤只处理明确白名单
.qwen和.qwen.root-orig：先修权限并普通删除，可用时尝试免密 sudo，再尝试将单个状态目录移入同文件系统 quarantine；如果 foreign-owned 目录本身无法跨父目录移动，最终才整体隔离 runner 自己拥有的 workspace，重建空 checkout 根目录并切换过去。.qwenignore等相似名称不匹配。为什么需要
现有
if: always()只能覆盖正常结束和普通失败。Worker 被直接终止时，.qwen.root-orig可能保持 root 所有且只读；下一次 checkout 因旧清理只处理.qwen而触发EACCES。Reviewer Test Plan / How to verify
验证 review、test、Web Shell smoke、integration job 都在 checkout 前恢复两个精确状态名，保留
.qwenignore；删除失败时先尝试单目录 quarantine，只有 foreign-owned 状态目录也无法移动时才整体隔离 workspace。按英文命令运行，预期 workflow cleanup 19/19。Evidence / Tested on
故障证据为 run 33146730771 / job 98770404551，在指定 ECS runner 上删除
.qwen.root-orig/agents时出现EACCES。修复后 cleanup 与 workflow-size suite 共 191 个测试通过（23 个按平台/能力跳过），ownership cleanup 子测试 2/2、Prettier 和 diff check 通过。Risk & Scope
范围仅限两个已知目录名；不改变 sandbox 策略、不新增主机权限。整体 workspace 隔离仅在权限修复、普通删除、sudo 删除和单目录 quarantine 都失败后触发，会丢弃该 runner 的热缓存但把原 workspace 保存在
_qwen-quarantine中供人工清理；整体隔离也失败时仍会明确告警。Linked Issues
关联上述 CI incident。