fix(ci): clean review worktrees after cancellation - #8474
Conversation
|
|
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Cancelled/timed-out reviews leaving Direction: aligned — CI reliability on reused self-hosted runners; a leaked worktree registration blocks unrelated PRs via "cannot delete branch ... used by worktree". Workflow-only change, no product surface. CHANGELOG not applicable. Size: 2 workflow files, +48/-0 (ci.yml +16, qwen-code-pr-review.yml +32). No core paths, no production code. Approach: scope feels right. Two layers matching the two failure modes: an Risk: no elevated risk signals — workflow files only, nothing matches the high-risk paths from the revert history. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实存在，不是理论问题。review 被取消/超时后会残留 方向：对齐——复用 self-hosted runner 上的 CI 可靠性问题，残留的 worktree 注册会以 "cannot delete branch ... used by worktree" 阻塞无关 PR。仅改 workflow，不涉及产品逻辑。 规模：2 个 workflow 文件，+48/-0（ci.yml +16，qwen-code-pr-review.yml +32）。不涉及核心路径和生产代码。 方案：范围合理。两层清理对应两种失败模式：review job 末尾的 风险：无升级风险信号——仅 workflow 文件，未命中 revert 历史中的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI'd have approached this the same way: there is no reliable way to run cleanup after a hard kill, so the answer is two independent layers — an Things I checked specifically:
Nothing blocking. One truly trivial nit: the ci.yml block lives inside a step named Test evidenceThis is a workflow-only change; the unit/integration suites don't exercise it. Two signals instead. First, this PR's own CI run executes the new code: same-repo branches route Not verified: a live cancellation-path run of the new Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Remaining entries on the head SHA ( 中文说明代码审查：方案与我的独立判断一致——硬 kill 之后无法可靠清理，所以用两层独立机制：review job 末尾的 重点核对：新步骤是 无阻塞问题。一个极小的命名 nit：ci.yml 的新逻辑放在名为 测试证据：仅 workflow 改动，单元/集成套件不会覆盖它。两个信号：其一，本 PR 自己的 CI 就在执行新代码——same-repo 分支的 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal fix that mirrors the repo's existing cleanup conventions; only a non-blocking naming nit and the inherent "first real cancellation proves it" gap remain. Stepping back: the two-layer design is exactly what this failure mode needs — a hard kill cannot run any cleanup, so an Reservations, minor: the step-name nit from the review above, and the fact that the cancellation path can't be exercised by any automated check — the first real cancelled review on a reused runner is the live test. Neither blocks. Approval deferred until CI lands green on 中文说明综合判断：两层设计正是这个失败模式需要的——硬 kill 无法执行任何清理，所以 保留意见（均不阻塞）：上面提到的步骤命名 nit；以及取消路径无法被任何自动化检查覆盖——第一次在复用 runner 上真实取消 review 就是它的现场验证。 批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
yiliang114
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-ci-bot
left a comment
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/ci.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.
中文说明
已审查。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/ci.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.3)
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8474Feedback point
There were no review comments, inline comments, or issue-level comments in this round's feedback; the failing CI check was the only item. Diagnosis (evidence trail)
Changes
No conflicts were encountered during the merge. No review threads existed to resolve or reply to. VerificationCommands actually run (this round, on the merged + fixed tree):
中文说明Autofix 本轮总结 — PR #8474反馈项
本轮反馈中没有评审意见、行内评论或 issue 级评论；失败的 CI 检查是唯一条目。 诊断（证据链）
变更内容
合并过程无冲突。本轮没有需要解决或回复的评审线程。 验证本轮实际执行的命令（在合并 + 修复后的树上）：
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/模型 |
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed — PR #8474Feedback source: one automated-reviewer Suggestion (R1-1), posted as two inline comments for the same finding: Finding R1-1 — duplicated review-artifact cleanup recipe with no pin to
|
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
|
🔀 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 将重新运行。 |
…worktrees # Conflicts: # scripts/tests/qwen-resolve-workflow.test.js
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： 🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8474Feedback points
There were no inline comments, no human or automated reviews, and no failed checks in this round's feedback. Conflict resolution
Resolution was decided against the actual workflow text, not by picking a side blindly:
All three hunks were resolved to main's version; the merged file is byte-identical to Changes
VerificationCommands actually run, in order:
中文说明Autofix 轮次总结 — PR #8474反馈点
本轮反馈中没有行内评论、没有人工或自动化审查、也没有失败的检查。 冲突解决
解决方式是依据实际工作流文本裁决，而不是盲目选边：
三个区块全部采用 main 的版本解决；合并后的文件与 改动
验证实际运行的命令（按顺序）：
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 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/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action needed (PR #8474)This round found no actionable feedback for PR #8474 ( What was checked:
PR state: the branch is up to date with its latest pushed commit ( No commit was created this round. 中文说明Autofix 审查轮次 — 无需操作（PR #8474）本轮未发现针对 PR #8474（ 已核查的内容：
PR 状态： 分支已包含最新推送的提交（ 本轮未创建任何提交。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix triage — PR #8474: no changes this roundThe only new feedback since the last evaluation is the failed Disposition:
|
|
Resolved the duplicate cleanup-extraction suggestions without code changes. I am keeping the workflow cleanup inline in this PR because extracting it into a shared script/action would broaden a targeted CI cleanup fix; that can be a separate follow-up if maintainers want it. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action needed (PR #8474)This round found no actionable feedback for PR #8474 ( What was checked (feedback newer than the last evaluation, 2026-08-04T06:41:37Z):
PR state: the branch is up to date with its latest pushed commit ( Verification (evidence gathering only; no code changed, nothing committed)
中文说明Autofix 审查轮次 — 无需操作（PR #8474）本轮未发现针对 PR #8474（ 已核查的内容（自上次评估 2026-08-04T06:41:37Z 之后的新反馈）：
PR 状态： 分支已包含最新推送的提交（ 验证（仅收集证据；未改动代码，未提交任何内容）
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Review @ Should fix
Nit
Note (no action needed)Running git pre-checkout against a leftover, untrusted |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary — PR #8474Addressed all actionable feedback from @wenshao's review at Feedback dispositions
Changed files
VerificationCommands actually run in this checkout (commit
中文说明Autofix 评审轮次总结 — PR #8474已处理 @wenshao 在 反馈处理结果
变更文件
验证在本次检出（提交
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/模型 |
Review —
|
Review:
|
| job | self-hosted | checkout | cleanup step |
|---|---|---|---|
test |
yes | yes | ✅ |
integration_cli |
yes | yes | ✅ (new) |
web_shell_e2e_smoke |
yes | yes | ❌ |
classify_pr |
yes | no | n/a |
The comment added to integration_cli — "this job lands on the same reused pool" — applies verbatim here. Either add the step or say why this job is exempt.
2. The review workflow's own pre-checkout sweep wasn't upgraded (medium). Clean stale agent state still uses the weaker rm -rf + prune recipe, and the new test encodes that weaker expectation ("Directories are rm -rf'd first there, so no worktree remove to pin"). But the E2/E3 fixture above shows the gap is real: if that rm -rf fails, the branch survives and the review's own git worktree add then hits missing but already registered / branch-in-use — the very failure this PR is about, one step earlier in the same workflow. Applying the same four-step recipe there would close the loop.
3. Duplication (low). The inline justification ("runs pre-checkout on shared runners, where leftover workspace files are untrusted") is sound for the two pre-checkout copies — there's no repo on disk to source a script from. It does not apply to Clean review worktrees, which runs post-checkout and could call a script in the repo. Not blocking; the test's "one identical awk filter at every list-driven sweep" assertion is a decent mitigation. Consider also asserting the two ci.yml copies are byte-identical, since they're meant to be.
4. timeout-minutes: 5 on an always() step (low). In the cancellation path GitHub grants a limited grace window before force-terminating the job; a 5-minute step budget may never be honored. The step is sub-second in practice — 1–2 minutes is a truer cap and leaves the runner room to finish.
5. The awk filter is substring-based (nit). index($0, "/.qwen/tmp/review-pr-") > 0 matches anywhere in the line, so it would also select the main checkout if the workspace path ever contained that substring. Harmless in practice (worktree remove on a main working tree just errors into a ::warning::), and the current runner layout is safe — but anchoring to $GITHUB_WORKSPACE would make "never the main checkout" structural rather than incidental. The test asserts it only for one hard-coded path.
6. Test coverage is structural, not behavioural (low). The suite pins strings and unit-tests the awk filter, but never executes the recipe — so ordering, quoting, or -e regressions slip through. qwen-pr-review-workflow.test.js already sets the precedent of extracting a run: block and executing the real bash against a fixture. A fixture test here (create worktree + branch, run the extracted block, assert branch gone and main checkout intact) would be materially stronger. Two smaller notes on the current test: expectCleanupRecipe uses run.indexOf('worktree remove --force'), which silently picks the first match if a second removal is ever added; and the it.skipIf(!awkAvailable) case is the only behavioural assertion, so on Windows the file degrades to pure string matching.
Security & performance
No concerns. Branch names come from for-each-ref and are quoted; a PR author can't create local qwen-review/* refs on the runner (checkout writes to refs/remotes/). rm -rf .qwen/tmp/review-pr-* is a relative glob under a cwd GHA pins to $GITHUB_WORKSPACE. Runtime is a handful of local Git calls — negligible, and it runs pre-checkout where it costs nothing.
Verdict
The change is correct, well-scoped, and the added complexity earns its place — I could measure the difference between the old and new recipes, which is not always true of CI hardening. Approve once #1 (the web_shell_e2e_smoke gap) is either fixed or explicitly ruled out of scope; #2 is the natural follow-up.
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Review feedback round — PR #8474Triaged the 2026-08-04 review by @wenshao (issue-level comment, four findings). Three findings fixed in code, one kept with counter-evidence. No merge performed ( Finding 1 —
|
| Finding | Disposition |
|---|---|
1 — web_shell_e2e_smoke coverage + spec inversion |
Fixed |
| 2 — lease prefix pinning | Fixed |
| 3 — comment undersells remove loop | Fixed |
4 — done || true no-op claim |
Kept (implicit bash -e makes it load-bearing) |
Changed files: .github/workflows/ci.yml, packages/cli/src/commands/review/lib/paths.ts, packages/cli/src/services/review-worktree-lease.ts, scripts/tests/review-worktree-cleanup-workflow.test.js. Commit: fix(ci): extend review cleanup sweep to web_shell_e2e_smoke (#8474).
Verification
npm run build— passednpm run typecheck— passednpm run lint— passednpx eslint scripts/tests/review-worktree-cleanup-workflow.test.js— passed (repolintscript only targets.ts/.tsx)npx vitest run --config ./scripts/tests/vitest.config.ts review-worktree-cleanup-workflow— 5/5 passed- Mutation checks on the spec —
LEASE_PREFIXrename,web_shell_e2e_smokestep removal, and web_shell awk-filter divergence each fail the spec; restored baseline is green cd packages/cli && npx vitest run src/services/review-worktree-lease.test.ts src/commands/review— 53 files passed, 1684 tests passed / 4 skippednpm run test:scripts— 896 passed / 9 skipped; 5 failures, allspawnSync zip ENOENTininstall-script.test.jsbecause this runner has nozipbinary (which zip→ not found,unzipexists). The tests shell out tozip; this diff touches neither them nor it. Environmental, not caused by this change- Structural YAML check (via the repo's
yamlparser) — all threeci.ymlsweep blocks byte-identical, each ordered before its job'sCheckout, no trailing whitespace yamllint/actionlint(node scripts/lint.js --yamllint/--actionlint) — not runnable on this runner (nopip3/binaries available); YAML validity confirmed via parser instead. CI runs both
中文说明
Review 反馈处理 — PR #8474
对 @wenshao 于 2026-08-04 提出的 review（issue 级评论，共四条发现）进行了分类处理。三条通过代码修复，一条以反证保留原状。未执行 merge（--conflict false）。
发现 1 — web_shell_e2e_smoke 未被清理逻辑覆盖 → 已修复
- 在
ci.yml的web_shell_e2e_smoke中，于Restore workspace ownership与Checkout之间加入了同样的Clean stale .qwen before checkout步骤——其run:块与test/integration_cli的配方逐字节一致（已用程序验证）。该 job 的 job 级if:已覆盖skip_ci，因此未添加步骤级条件，与integration_cli保持一致。 - 按建议反转了 spec 的枚举方式：现在通过
runs-on包含ubuntu_runner且步骤中使用actions/checkout来过滤ci.yml的 job，并要求每个匹配的 job 都有清理步骤及完整配方。arrayContaining(['test', 'web_shell_e2e_smoke', 'integration_cli'])断言防止过滤器静默匹配不到任何 job。awk 过滤器一致性测试也改为遍历同一枚举，因此任何池中 job 的过滤器不一致都会失败。 - 变异测试验证：删除/重命名
web_shell_e2e_smoke的步骤会以job "web_shell_e2e_smoke" checks out on the shared pool and must clean stale .qwen state first失败；使其 awk 过滤器不一致会让一致性测试失败。 - review 中点名的其他同池工作流（
npm-cache.yml:save、sdk-java.yml、serve-ab.yml）被明确标注为超出本 PR 范围，未改动。
发现 2 — spec 未固定 lease 文件名 → 已修复
- 将
LEASE_PREFIX从review-worktree-lease.ts移入packages/cli/src/commands/review/lib/paths.ts（spec 本就视该模块为布局的唯一来源；review-worktree-lease.ts本来就从该模块导入），并将其导出。 - 在 review-job 测试中新增
expect(reviewCleanStep).toContain(\rm -f ${toPosix(REVIEW_TMP_DIR)}/${LEASE_PREFIX}pr-*.json`)`，使工作流中的 glob 与 lease 写入方共享同一个被固定的定义。 - 变异测试验证：重命名
LEASE_PREFIX会使 spec 失败。
发现 3 — 注释低估了 remove 循环的作用 → 已修复
- 重写了
ci.yml三处副本中的注释：现在说明prune会清除已被rm删除的目录对应的登记项，worktree remove --force负责清理在 rm 之后仍然残留的目录（root 属主或只读残留），并且分支只有在没有 worktree 占用时才能被删除。 - spec 中的顺序断言现在会先剔除注释行再检查命令顺序，因为改进后的注释确实会以非命令顺序提及
worktree remove --force。
发现 4 — qwen-code-pr-review.yml:404 的 done || true 被认为是空操作 → 保留，附证据
- 保持原状。GitHub Actions 以
bash -e {0}调用run:步骤，因此即使该步骤没有写set -e，-e也是生效的（仓库中的set -uo pipefail只是在那个隐式-e之上追加-u/pipefail）。在-e+pipefail下，git for-each-ref一旦失败，管道即返回非零并在脚本中途中止该步骤，导致 job 失败——这正是该步骤"绝不使 job 失败"约定所禁止的。此处的|| true也与该步骤中其他每条命令的守卫以及新增块保持一致。review 本身也明确允许保留（"无害，为了对称性保留也可以"），因此本轮不做代码改动。
处理结果
| 发现 | 处理 |
|---|---|
1 — web_shell_e2e_smoke 覆盖 + spec 反转 |
已修复 |
| 2 — lease 前缀固定 | 已修复 |
| 3 — 注释低估 remove 循环 | 已修复 |
4 — done || true 空操作的说法 |
保留（隐式 bash -e 使其具有实际作用） |
变更文件：.github/workflows/ci.yml、packages/cli/src/commands/review/lib/paths.ts、packages/cli/src/services/review-worktree-lease.ts、scripts/tests/review-worktree-cleanup-workflow.test.js。提交：fix(ci): extend review cleanup sweep to web_shell_e2e_smoke (#8474)。
验证
-
npm run build— 通过 -
npm run typecheck— 通过 -
npm run lint— 通过 -
npx eslint scripts/tests/review-worktree-cleanup-workflow.test.js— 通过（仓库的lint脚本只针对.ts/.tsx） -
npx vitest run --config ./scripts/tests/vitest.config.ts review-worktree-cleanup-workflow— 5/5 通过 - spec 变异检查 — 重命名
LEASE_PREFIX、删除web_shell_e2e_smoke步骤、使 web_shell 的 awk 过滤器不一致，均会使 spec 失败；恢复后的基线为绿色 -
cd packages/cli && npx vitest run src/services/review-worktree-lease.test.ts src/commands/review— 53 个文件通过，1684 个测试通过 / 4 个跳过 -
npm run test:scripts— 896 通过 / 9 跳过；5 个失败，全部是install-script.test.js中的spawnSync zip ENOENT，原因是本 runner 没有zip可执行文件（which zip→ 未找到，unzip存在）。这些测试通过 shell 调用zip；本 diff 既未改动这些测试也未改动zip。属于环境问题，与本次变更无关 - 结构化 YAML 检查（使用仓库的
yaml解析器）—ci.yml中三处清理块逐字节一致，且各自都排在其 job 的Checkout之前，无行尾空白 -
yamllint/actionlint（node scripts/lint.js --yamllint/--actionlint）— 本 runner 上无法运行（无pip3/对应二进制）；已改用解析器确认 YAML 有效性。CI 会运行这两项
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/模型 qwen3.8-max
|
@qwen-code-review-bot approve if no blockers |
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
LGTM — all of @wenshao's findings are addressed at this head. Verified f7f0954: web_shell_e2e_smoke now carries the Clean stale .qwen before checkout step (Restore workspace ownership → sweep → Checkout), closing the conditional from the last review, and the spec's inverted enumeration means the next job added to the shared pool fails loudly instead of silently. Lease-prefix pinning and the pipeline || true guards are in as well.
CI at head is green, including web-shell E2E Smoke which executes the new sweep; review-pr (automated LLM review) is still running but is non-blocking per the repo's own autofix config and has posted no findings across two infra-timeout runs — the human review here is the substantive one.
Remaining items (other pool workflows, upgrading the review workflow's own pre-checkout recipe, behavioural fixture test were explicitly marked follow-up by the reviewer. Ship it.
EOF
)
Review:
|
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Review feedback round — PR #8474Triaged the 2026-08-04 follow-up review by @wenshao (issue-level comment, six findings) plus @yiliang114's approval trigger. No code changes this round: the review's only merge-blocking finding (#1) was already fixed one commit before the review was posted, and the remaining findings are non-blocking follow-ups per the review's own verdict. No merge performed ( Context: the review crossed with the previous pushThe review (posted 14:12 UTC) was written against the pre- Finding 1 —
|
|
Addressed the high-security cleanup finding in commit I did not expand this PR to other workflows or runner-hook consolidation; those are follow-up scope. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Address review summary — PR #8474 (round on top of
|
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
.github/workflows/ci.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.
中文说明
.github/workflows/ci.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。
— claude-opus-4-6 via Qwen Code /review (v0.21.5)
Qwen Code Review — PR #8474Title: fix(ci): clean review worktrees after cancellation Build & test: Build green (15 workspaces). 0 net-new test failures. New cleanup workflow test 5/5 pass. Findings:
Previously reported blockers re-checked:
Verdict: Comment (base verdict was Approve, downgraded to Comment because target CI is still running). Review run completed at 2026-08-05. Note: the qwen review process logged internal tooling noise ( |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8474One commit this round: Feedback triage1. Suggestion from the automated review summary — IMPLEMENTED.
Verified first: the three Proved the assertion works both ways:
2. Review downgraded Approve → Comment because CI was still running — no code action. Informational; CI status is resolved by the workflow's own checks, not by a code change. 3. "Not linted: actionlint embedded-shell source mapping is not yet supported" — no code action. The reviewer explicitly marks this as a tool limitation, not a blocker; there is no repository defect to fix. The embedded shell is exercised by the Vitest workflow test instead (recipe content, ordering, hardened git calls, awk filter semantics). 4. Re-verified previously reported blockers (hooks-path security, No inline comments existed this round, so there are no resolved threads and no reply comments. Verification
中文说明Autofix 审查轮次 — PR #8474本轮一个提交： 反馈分类处理1. 自动审查摘要中的 Suggestion — 已实现。
先做了核实： 双向验证了该断言的有效性：
2. 审查从 Approve 降级为 Comment，原因是 CI 仍在运行 — 无需代码改动。 仅为状态说明；CI 状态由工作流自身的检查决定，不需要代码变更。 3. “未做 lint：actionlint embedded-shell source mapping 尚不支持” — 无需代码改动。 审查者明确标注这是工具限制、非阻断项；仓库中不存在需要修复的缺陷。嵌入式 shell 改由 Vitest 工作流测试覆盖（配方内容、顺序、加固后的 git 调用、awk 过滤器语义）。 4. 复核此前提出的阻断项（hooks-path 安全问题、 本轮没有任何行内评论，因此没有可解决的讨论串，也没有需要回复的评论。 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 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; 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/ci.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.5)
| expect(run).not.toMatch( | ||
| /^\s+git(?:\s+-C\s+"\$GITHUB_WORKSPACE")?\s+(?:worktree|for-each-ref|branch)\b/m, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R2-1: The hardened-git regression check requires leading whitespace (^\s+), so it covers only the indented ci.yml sweep copies — the two review-workflow sweep steps, whose git calls land at column 0 after YAML block-scalar stripping, are not covered. The verb whitelist (worktree|for-each-ref|branch) additionally lets a future column-0 bare git status escape. — Failure scenario: mutation-proven → reverting "${GIT_SAFE[@]}" worktree prune -v || true (qwen-code-pr-review.yml:1009) to bare git worktree prune leaves the suite 5/5 green; an un-hardened call would then run git against leftover untrusted .git config on the shared self-hosted runner, where core.hooksPath/core.fsmonitor are code-execution vectors (a bare branch -D was proven to fire a leftover reference-transaction hook).
| expect(run).not.toMatch( | |
| /^\s+git(?:\s+-C\s+"\$GITHUB_WORKSPACE")?\s+(?:worktree|for-each-ref|branch)\b/m, | |
| ); | |
| expect(run).not.toMatch(/^\s*git\s/m); |
The GIT_SAFE=(git … definition line starts with GIT_SAFE=(, so the relaxed anchor cannot false-positive; verified to flag the mutation with zero false positives on the current scripts.
中文说明
加固 git 的回归检查要求行首有空白（^\s+），因此只能覆盖有缩进的 ci.yml 清扫副本；两个 review-workflow 清扫步骤的 git 调用在 YAML 块标量剥离后位于第 0 列，不会被匹配到。动词白名单（worktree|for-each-ref|branch）还会让未来第 0 列的裸 git status 漏网。失败场景：已通过变异证实——把 qwen-code-pr-review.yml:1009 的 "${GIT_SAFE[@]}" worktree prune -v || true 还原为裸 git worktree prune，测试套件仍为 5/5 全绿；未加固的调用会在共享 self-hosted runner 上对不受信任的残留 .git 配置执行 git，而 core.hooksPath/core.fsmonitor 是代码执行入口（裸 branch -D 已证实会触发残留的 reference-transaction 钩子）。建议的 /^\s*git\s/m 不会误报（GIT_SAFE=(git … 定义行以 GIT_SAFE=( 开头），并已验证能捕获该变异。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| | while read -r stale_ref; do | ||
| if [ -n "$stale_ref" ]; then | ||
| git branch -D "$stale_ref" || true | ||
| "${GIT_SAFE[@]}" branch -D "$stale_ref" || true |
There was a problem hiding this comment.
[Suggestion] R2-2: A branch-delete failure is silently swallowed here (|| true), while the three ci.yml copies and the new Clean review worktrees step added by this PR emit ::warning::could not remove review branch: … for the identical failure. — Failure scenario: if branch -D fails in this pre-review sweep (git lock contention, permissions, or a worktree the preceding rm -rf could not delete keeping the branch checked out) → the leftover qwen-review/pr-N branch survives with zero log evidence, and the next review of that PR wedges with no warning pointing at the run that left the branch behind.
| "${GIT_SAFE[@]}" branch -D "$stale_ref" || true | |
| "${GIT_SAFE[@]}" branch -D "$stale_ref" || echo "::warning::could not remove review branch: $stale_ref" |
中文说明
此处删除分支失败被静默吞掉（|| true），而三份 ci.yml 副本和本 PR 新增的 Clean review worktrees 步骤对同样的失败都会输出 ::warning::could not remove review branch: …。失败场景：如果这次 review 前清扫中 branch -D 失败（git 锁竞争、权限、或前面的 rm -rf 未能删除、仍检出该分支的 worktree）→ 残留的 qwen-review/pr-N 分支会存活且日志中毫无痕迹，该 PR 的下一次 review 会因此卡住，且没有任何警告指向留下分支的那次运行。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const reviewCleanStep = reviewYaml.jobs['review-pr'].steps.find( | ||
| (s) => s.name === 'Clean review worktrees', | ||
| ).run; |
There was a problem hiding this comment.
[Suggestion] R2-3: This extracts only .run from the Clean review worktrees step and never pins the if: 'always()' condition or the end-of-job position — the very properties that make the step fire on the failure/cancellation paths it exists for. — Failure scenario: mutation-verified → deleting if: 'always()' from the workflow leaves all 5 tests green, yet without it Actions' default success() condition skips the step whenever any earlier step of review-pr fails or the job is cancelled/timed out — exactly the interrupted-review scenario this PR exists to sweep — and that regression ships with a green test:ci.
const reviewCleanSteps = reviewYaml.jobs['review-pr'].steps;
const reviewCleanIndex = reviewCleanSteps.findIndex(
(s) => s.name === 'Clean review worktrees',
);
const reviewCleanStep = reviewCleanSteps[reviewCleanIndex].run;
// then, inside the test:
expect(reviewCleanSteps[reviewCleanIndex].if).toBe('always()');
expect(reviewCleanIndex).toBe(reviewCleanSteps.length - 1);中文说明
这里只取出 Clean review worktrees 步骤的 .run，从未钉住 if: 'always()' 条件和“位于 job 末尾”这两个属性——而正是这两个属性让该步骤在其为之存在的失败/取消路径上执行。失败场景：已变异验证——删除 workflow 中的 if: 'always()' 后 5 个测试仍全绿；而缺少它时，Actions 默认的 success() 条件会在 review-pr 任何先前步骤失败或 job 被取消/超时时跳过该步骤——正是本 PR 要清扫的中断场景——该回归会在 test:ci 全绿的情况下上线。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| "${GIT_SAFE[@]}" worktree list --porcelain \ | ||
| | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \ | ||
| | while read -r worktree; do |
There was a problem hiding this comment.
[Suggestion] R2-4: The sweep feeds attacker-influenceable leftover git metadata (.git/worktrees/*/gitdir contents) unnormalized into destructive git worktree remove --force; git resolves .. traversal in the registered path, so the “delete only review artifacts” guarantee rests on a substring check over an untrusted string. The three ci.yml sweep copies share the identical recipe. — Failure scenario: reproduced end-to-end → a prior job with runner-user code execution rewrites .git/worktrees/<id>/gitdir to <ws>/.qwen/tmp/review-pr-42/../../../../target/.git and plants the backlink; the awk filter matches the raw string, git backlink validation passes, and worktree remove --force deletes the attacker-chosen target directory outside the review tree. Impact is bounded (the attacker already has runner-user code execution — no privilege escalation), but the cleanup becomes a delayed, plausibly-denied arbitrary-directory-deletion primitive.
# in each loop iteration, before `worktree remove` (all four recipe copies):
case "$worktree" in
*/../*|../*|*/..)
echo "::warning::skipping suspicious review worktree path: $worktree"
continue
;;
"$GITHUB_WORKSPACE/.qwen/tmp/review-pr-"*) : ;;
*)
echo "::warning::skipping unexpected review worktree path: $worktree"
continue
;;
esacVerified on a fixture: the guard skips the traversal path with a warning, the target survives, and legitimate leftover review worktrees are still removed.
中文说明
清扫把可被攻击者影响的残留 git 元数据（.git/worktrees/*/gitdir 内容）未经规范化就喂给破坏性的 git worktree remove --force；git 会解析注册路径中的 .. 穿越，因此“只删除 review 产物”的保证仅依赖对不受信任字符串的子串检查。三份 ci.yml 清扫副本的配方完全相同。失败场景：已端到端复现——先前拥有 runner 用户代码执行权限的任务把 .git/worktrees/<id>/gitdir 改写为 <ws>/.qwen/tmp/review-pr-42/../../../../target/.git 并植入回链；awk 过滤器匹配原始字符串，git 回链校验通过，worktree remove --force 删除了 review 目录之外、由攻击者选定的 target 目录。影响有界（攻击者已拥有 runner 用户代码执行权限——无提权），但清理变成了一个延迟的、可抵赖的任意目录删除原语。修复已用 fixture 验证：守卫跳过穿越路径并输出警告，目标目录幸存，合法的残留 review worktree 仍会被移除。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const ciCleanSteps = Object.entries(ciYaml.jobs) | ||
| .filter( | ||
| ([, job]) => | ||
| JSON.stringify(job['runs-on'] ?? '').includes('ubuntu_runner') && |
There was a problem hiding this comment.
[Suggestion] R2-5: This matches the literal output-reference name ubuntu_runner, not the pool itself, so a future ci.yml checkout job that hard-codes the shared-pool label array (fromJSON('["self-hosted", "linux", "x64", "ecs-qwen"]') — the repo's dominant routing pattern, 27 occurrences across 16 workflows) silently escapes the pinning. — Failure scenario: a future ci.yml job lands on the shared pool via the hard-coded form and checks out → this filter never requires the cleanup step for it, so that job hits exactly the interrupted-review leftover this PR exists to remove — the very checkout failure the test comment claims cannot happen. Coverage is complete today (the three ubuntu_runner-output jobs are all swept); the cost is false confidence in the enumeration.
| JSON.stringify(job['runs-on'] ?? '').includes('ubuntu_runner') && | |
| /ubuntu_runner|ecs-qwen/.test(JSON.stringify(job['runs-on'] ?? '')) && |
Safe today: classify_pr also matches ecs-qwen but has no actions/checkout step, so the enumerated set stays the same three jobs.
中文说明
这里匹配的是字面输出引用名 ubuntu_runner 而非池本身；未来某个硬编码共享池标签数组（fromJSON('["self-hosted", "linux", "x64", "ecs-qwen"]')——仓库中最常见的路由模式，16 个 workflow 中有 27 处）的 ci.yml checkout 任务会静默逃脱该钉住。失败场景：未来某个 ci.yml 任务以硬编码形式落在共享池并执行 checkout → 此过滤器不会要求它带清理步骤，该任务就会撞上本 PR 要清除的中断-review 残留——正是测试注释声称不可能发生的 checkout 失败。当前覆盖是完整的（三个 ubuntu_runner 输出任务均已清扫）；代价是枚举带来的虚假信心。建议的写法今天是安全的：classify_pr 也匹配 ecs-qwen，但它没有 actions/checkout 步骤，枚举出的集合仍是同样三个任务。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| expect(code.indexOf('worktree prune', remove)).toBeGreaterThan(remove); | ||
| expect(code.indexOf(`refs/heads/${branchFamily}*`)).toBeGreaterThan(remove); |
There was a problem hiding this comment.
[Suggestion] R2-6: The suite never pins the trailing || true on the piped sweep loops, so the steps' failure-isolation contract (a cleanup failure degrades to ::warning::, never fails the job) is mutation-invisible in every sweep step — including the done || true this diff newly adds to Clean stale agent state (qwen-code-pr-review.yml:408), which expectCleanupRecipe does not cover at all. — Failure scenario: mutation-proven both arms → dropping || true from the Clean review worktrees branch loop, and from all three ci.yml branch loops at once, both leave the suite 5/5 green (byte-identity only compares copies to each other). Under set -uo pipefail + bash -e, a failing for-each-ref/worktree list head — the corrupt-leftover state these steps exist to tolerate — then aborts the step: the pre-checkout gate goes red before checkout, or the always() step marks review-pr failed after the review comment was already posted.
// in expectCleanupRecipe (and applied to the agent-state step too):
const loops =
code.match(/\|\s*while read -r \w+; do[\s\S]*?\n\s*done(?: \|\| true)?/g) ?? [];
expect(loops.length).toBeGreaterThanOrEqual(2);
for (const loop of loops) {
expect(loop.endsWith('done || true')).toBe(true);
}Verified: the assertion passes on the current YAML and fails both mutations.
中文说明
套件从未钉住管道清扫循环末尾的 || true，因此失败隔离契约（清理失败只降级为 ::warning::，绝不失败 job）在所有清扫步骤中都是变异不可见的——包括本 diff 新加给 Clean stale agent state 的 done || true（qwen-code-pr-review.yml:408），expectCleanupRecipe 完全没有覆盖该步骤。失败场景：双向变异均已证实——删掉 Clean review worktrees 分支循环的 || true，或同时删掉三份 ci.yml 分支循环的 || true，套件都保持 5/5 全绿（逐字节一致断言只在副本之间互相比较）。在 set -uo pipefail + bash -e 下，for-each-ref/worktree list 头部失败——正是这些步骤要容忍的损坏残留状态——会中止该步骤：checkout 前的门禁变红，或 always() 步骤在 review 评论已发出后把 review-pr 标记为失败。已验证：该断言在当前 YAML 上通过，并使两种变异都失败。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| .map(([id, job]) => ({ | ||
| id, | ||
| run: job.steps.find((s) => s.name === 'Clean stale .qwen before checkout') |
There was a problem hiding this comment.
[Suggestion] R2-7: This pins the ci.yml clean step's content but never its position — it must run before Checkout and after Restore workspace ownership, and the map here discards the index that would prove it. — Failure scenario: mutation-proven → moving the clean step after Checkout in web_shell_e2e_smoke leaves the suite 5/5 green; on a dirty reused runner, checkout then hits the leftover worktree/branch registrations first — the exact “cannot delete branch … used by worktree” failure this PR fixes — and the sweep no-ops on the freshly checked-out tree. Moving it before Restore workspace ownership also passes green, yet git refuses root-owned leftover repos (“detected dubious ownership”), and every sweep git call fails silently under || true.
// keep the steps array in the mapped entries, then per matched job:
const cleanIdx = steps.findIndex(
(s) => s.name === 'Clean stale .qwen before checkout',
);
const checkoutIdx = steps.findIndex((s) =>
String(s.uses ?? '').includes('actions/checkout'),
);
const restoreIdx = steps.findIndex(
(s) => s.name === 'Restore workspace ownership',
);
expect(cleanIdx).toBeLessThan(checkoutIdx);
expect(cleanIdx).toBeGreaterThan(restoreIdx);中文说明
这里钉住了 ci.yml 清理步骤的内容，却从未钉住其位置——它必须在 Checkout 之前、Restore workspace ownership 之后运行，而此处的 map 丢弃了能证明这一点的索引。失败场景：已变异证实——把 web_shell_e2e_smoke 中的清理步骤移到 Checkout 之后，套件仍 5/5 全绿；在复用的脏 runner 上，checkout 会先撞上残留的 worktree/分支注册——正是本 PR 修复的 “cannot delete branch … used by worktree” 失败——随后清扫对刚检出的目录变成空操作。移到 Restore workspace ownership 之前同样全绿，但 git 会拒绝 root 拥有的残留仓库（“detected dubious ownership”），清扫的每个 git 调用都会在 || true 下静默失败。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| "${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \ | ||
| | while read -r review_ref; do | ||
| [ -n "$review_ref" ] || continue |
There was a problem hiding this comment.
[Suggestion] R2-8: The sweep only removes worktrees whose registered path contains /.qwen/tmp/review-pr-; a leftover registration elsewhere that holds a qwen-review/* branch checked out is never removed, making that branch permanently undeletable on the runner. The three ci.yml copies share the identical pinned filter. — Failure scenario: reproduced with real git → a holder worktree registered outside the filter prefix, checking out qwen-review/pr-99: the awk filter skips it, worktree prune keeps it (directory exists), and branch -D fails with this PR's verbatim motivating error on every run, emitting only ::warning::; nothing ever removes the holder, so every subsequent checkout on that runner degrades the same way. The repo's own tooling cannot produce this state (all three worktree add sites match the filter); the trigger is another actor with runner-user code execution — the same bounded prerequisite class as the traversal finding.
# on branch -D failure, find the holder by ref, remove it, retry:
holder=$("${GIT_SAFE[@]}" worktree list --porcelain \
| awk -v ref="refs/heads/$review_ref" \
'$1 == "worktree" { wt = $2 } $1 == "branch" && $2 == ref { print wt }')
if [ -n "$holder" ]; then
"${GIT_SAFE[@]}" worktree remove --force "$holder" 2>/dev/null || rm -rf "$holder" 2>/dev/null || true
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" branch -D "$review_ref" ||
echo "::warning::could not remove review branch: $review_ref"
fiVerified on a scratch repo: the fixed sweep deletes the branch and leaves only the main worktree.
中文说明
清扫只移除注册路径包含 /.qwen/tmp/review-pr- 的 worktree；检出了 qwen-review/* 分支、却注册在别处的残留 worktree 永远不会被移除，该分支在 runner 上永久无法删除。三份 ci.yml 副本共用同一被钉住的过滤器，缺口相同。失败场景：已用真实 git 复现——过滤器前缀之外注册的 holder worktree 检出 qwen-review/pr-99：awk 过滤器跳过它，worktree prune 保留它（目录存在），branch -D 每次运行都以本 PR 要修复的原错误失败，只输出一条 ::warning::；没有任何东西移除 holder，该 runner 上后续每次 checkout 都以同样方式退化。仓库自身工具不会产生这种状态（三处 worktree add 均匹配过滤器）；触发需要另一个拥有 runner 用户代码执行权限的参与者——与路径穿越发现同属有界前提类别。已在临时仓库验证：修复后的清扫能删除该分支，最终只剩主工作树。
— qwen3.8-max via Qwen Code /review (v0.21.5)
There was a problem hiding this comment.
Declined for this round, with thanks for the thorough reproduction. The holder state this targets cannot be produced by this repo's own tooling — all three worktree add sites match the pinned filter, and the finding itself bounds the trigger to another actor with runner-user code execution (the same bounded prerequisite class as the traversal finding). Two concrete costs tipped the call: the retry ladder would add ~32 lines across the four test-pinned byte-identical recipe copies, and its rm -rf "$holder" operates on exactly the untrusted registered path the traversal finding warns about — it would itself need the path guard landed in this round before it could ship. Meanwhile the failure stays visible and recoverable: every affected run emits ::warning::could not remove review branch: … naming the branch, so a one-time manual git worktree remove clears it. The direction we did take narrows the sweep's destructive guarantees (prefix + traversal guard) rather than widening its reach for a state we cannot produce. Happy to revisit with the guard composed in if a maintainer wants the holder recovery regardless.
中文说明
本轮暂不采纳，感谢详尽的复现。该建议针对的 holder 状态无法由本仓库自身工具产生——三处 worktree add 调用点都匹配被钉住的过滤器，且该发现本身将触发条件限定为另一个拥有 runner 用户代码执行权限的参与者（与路径穿越发现同属有界前提类别）。有两个具体成本促成了这一决定：重试阶梯需要在四份被测试钉住逐字节一致的配方副本中各增加约 32 行；且其中的 rm -rf "$holder" 操作的正是路径穿越发现所警告的不受信任注册路径——它自己必须先套上本轮落地的路径守卫才能上线。与此同时，该失败始终可见且可恢复：每次受影响的运行都会输出点名分支的 ::warning::could not remove review branch: …，一次手动 git worktree remove 即可清除。我们选择的方向是收窄清扫的破坏性保证（前缀 + 穿越守卫），而不是为我们无法产生的状态扩大其触及范围。如果维护者仍希望加上 holder 恢复逻辑，把它与守卫组合起来后我们很乐意再议。
|
🤖 Addressed the latest review feedback (round 8/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 8/100 轮）。改动内容与我反驳保留之处如下： PR #8474 — review round summaryRound 2 feedback: 8 automated-reviewer suggestions (R2-1 … R2-8). 7 implemented, 1 declined with reason. One commit on the existing head branch: Feedback dispositionsR2-1 — implemented (rc:3716522357)The hardened-git regression check required leading whitespace ( R2-2 — implemented (rc:3716522389)The agent-state sweep's R2-3 — implemented (rc:3716522400)The suite now pins R2-4 — implemented (rc:3716522403)Added a path guard in all four worktree-removal loops (three ci.yml copies — kept byte-identical, verified — plus the review-workflow copy), before the destructive
This closes the reproduced end-to-end traversal hole: the awk filter matches a substring of an untrusted registered path, and git resolves R2-5 — implemented (rc:3716522405)The shared-pool enumeration now matches the pool itself — R2-6 — implemented (rc:3716522409)New R2-7 — implemented (rc:3716522418)The mapped ci.yml entries now keep the steps array, and each matched job asserts the clean step's position: after R2-8 — declined (rc:3716522421)The holder-recovery ladder targets a state this repository's own tooling cannot produce — every VerificationCommands actually run and their results:
中文说明PR #8474 — review 轮次总结第 2 轮反馈：自动评审器 8 条建议（R2-1 … R2-8）。实现 7 条，附理由拒绝 1 条。在现有 head 分支上提交一次： 反馈处置R2-1 — 已实现（rc:3716522357）加固 git 的回归检查原先要求行首有空白（ R2-2 — 已实现（rc:3716522389）agent-state 清扫中的 R2-3 — 已实现（rc:3716522400）套件现在钉住 R2-4 — 已实现（rc:3716522403）在全部四处 worktree 移除循环（三份 ci.yml 副本——已验证保持逐字节一致——加 review-workflow 副本）中，在破坏性的
这堵上了已端到端复现的穿越漏洞：awk 过滤器匹配的是不受信任注册路径的子串，而 git 会解析 R2-5 — 已实现（rc:3716522405）共享池枚举现在匹配池本身—— R2-6 — 已实现（rc:3716522409）新增 R2-7 — 已实现（rc:3716522418）映射出的 ci.yml 条目现在保留 steps 数组，每个匹配任务都断言清理步骤的位置：在 R2-8 — 已拒绝（rc:3716522421）holder 恢复阶梯针对的状态无法由本仓库自身工具产生——所有 验证实际运行的命令及结果：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Approving, verified against the current HEAD f19e3fa rather than the SHA I started on — the branch moved mid-review and the earlier hardening notes are already addressed there.
What I checked at f19e3fa:
- All git invocations in the sweep now go through
GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE"), in all threeci.ymlcopies and bothqwen-code-pr-review.ymlsweeps, so acore.hooksPathplanted in a leftover untrusted.gitno longer fires onbranch -D/worktree remove. - The block comment no longer claims
worktree remove --forceclears root-owned leftovers; it now describes the real value (deregistering sobranch -Dcan succeed even when the directory survives). - The workflow-parsing test guards the optional keys (
job['runs-on'] ?? '',(job.steps ?? [])), so a future job-leveluses:job can't turn the enumeration into a TypeError. I confirmed the broadenedubuntu_runner|ecs-qwenfilter does not accidentally pull inclassify_pr: it matches the label array but has noactions/checkoutstep, so it is correctly excluded rather than silently failing the recipe assertions. - Cancellation coverage is real: the cleanup step is
if: always()and is the last step ofreview-pr, and the job-levelifis evaluated at job start, so a mid-flight cancellation still reaches it. - Blast radius stays scoped to
.qwen/tmp/review-pr-*, localrefs/heads/qwen-review/*, and theqwen-review-lease-pr-*.jsonleases; theqwen-review-pr-*sibling prefix is deliberately not matched byreview-pr-*. - Every added command is
|| true-guarded at the pipeline level, so the "never fail the job" property holds under the workflow's-e/pipefaildefaults. - CI is green on this exact SHA: run
30959526003hashead_sha=f19e3fa,conclusion=success, andTest (ubuntu-latest, Node 22.x)passed, which is the leg that runs the newscripts/testssuite.
No blocking issues from me.
|
Released in v0.21.6. |
6 participants
What this PR does
Adds an always-run cleanup step to the PR review job and extends the CI pre-checkout cleanup to remove stale Git worktree registrations and qwen-review branches on reused self-hosted runners.
Why it's needed
Cancelled or timed-out reviews can leave review worktrees and branch registrations behind. A later CI checkout then fails with "cannot delete branch ... used by worktree", blocking unrelated PRs on the same runner.
Reviewer Test Plan
How to verify
Confirm that the review job cleanup runs with
always()after success, failure, cancellation, and the internal review timeout path. Confirm that the CI pre-checkout step prunes missing worktrees, removes review worktrees, and deletes onlyqwen-review/*branches before checkout.Evidence (Before & After)
N/A — workflow-only change.
Tested on
Environment (optional)
node scripts/lint.js --actionlintandnode scripts/lint.js --yamllintpassed.Risk & Scope
.qwen/tmp/review-pr-*worktrees andqwen-review/*branches.Linked Issues
No linked issue.
中文说明
本 PR 做了什么
为 PR review job 增加始终执行的最终清理步骤，并扩展 CI checkout 前的清理逻辑，处理复用 self-hosted runner 上残留的 Git worktree 注册和 qwen-review 分支。
为什么需要
review 被取消或超时后，可能会留下 review worktree 和分支注册。后续 CI checkout 会报“cannot delete branch ... used by worktree”，从而阻塞同一 runner 上的其他 PR。
Reviewer Test Plan
如何验证
确认 review 成功、失败、取消以及内部 review 超时后，清理步骤都会通过
always()执行；确认 CI checkout 前会清理缺失的 worktree、移除 review worktree，并且只删除qwen-review/*分支。证据（前后对比）
不适用——仅修改 workflow。
测试环境
Linux：已通过
node scripts/lint.js --actionlint和node scripts/lint.js --yamllint。风险与范围
.qwen/tmp/review-pr-*worktree 和qwen-review/*分支。关联 Issue
无。