docs(ci): explain why the review-pr sweep needs a whole-workspace quarantine - #10493
docs(ci): explain why the review-pr sweep needs a whole-workspace quarantine#10493CanReader wants to merge 1 commit into
Conversation
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@CanReader thanks for the PR — but the gap it describes is already fixed on main, and the diff doesn't match the description.
- The fix already landed. #10214 (merged today 10:33 UTC as commit
379bef96) added exactly this sweep to thereview-prjob: theClean stale .qwen before checkoutstep running thechmod → rm → sudo rm → quarantineloop over both.qwenand.qwen.root-orig, placed beforeCheckout base branch— plus the structural test pin assertingcleanIdx < checkoutIdxinscripts/tests/review-worktree-cleanup-workflow.test.js. #10214's description said its diff was scoped to the threeci.ymlcopies, but the merged PR also modifiedqwen-code-pr-review.yml(+47 lines). Everything #10392 asks for is already on the default branch; the issue is simply still open. - The actual diff here is comment-only. The entire change is a 3-line → 6-line rewording of the quarantine-fallback comment inside the sweep (+6/−3, zero functional change). The sweep and the test the description references are not added by this PR — they are already in the base.
- Why request changes: merging as-is would auto-close #10392 via a PR that doesn't contain the fix, misattributing the history. Two reasonable paths forward:
中文说明
@CanReader 感谢提交 —— 但这个 PR 描述的缺口已经在 main 上修复了,而且 diff 与描述不符。
- 修复已经合入。 #10214(今天 10:33 UTC 合入,commit
379bef96)已经把这个清理逻辑加进了review-pr任务:Clean stale .qwen before checkout步骤,对.qwen和.qwen.root-orig两个名字执行chmod → rm → sudo rm → quarantine循环,位于Checkout base branch之前 —— 同时还在scripts/tests/review-worktree-cleanup-workflow.test.js中加了断言cleanIdx < checkoutIdx的结构测试。虽然 #10214 的描述说它只改了三份ci.yml,但合入的版本实际也修改了qwen-code-pr-review.yml(+47 行)。#10392 要求的内容已经全部在默认分支上,issue 只是还没被关闭。 - 这里的实际 diff 只是注释改写。 全部改动是把 sweep 中 quarantine 兜底路径的 3 行注释改写成 6 行(+6/−3,零功能变化)。描述中提到的 sweep 和结构测试都不是这个 PR 加的 —— 它们已经在 base 里了。
- 为什么 request changes: 按现状合入会通过一个不包含修复的 PR 自动关闭 #10392,造成历史归属错误。两个合理的处理方式:
— Qwen Code · qwen3.8-max
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6b": none — no check was cut short by the tool ceiling..
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未探索到全部深度(达到工具调用预算):"agent 6b":none — no check was cut short by the tool ceiling.。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.3)
…rantine The old comment stated the fallback but not the reason it exists, which makes it read as redundant next to the mv right above it. Renaming a directory across parents also needs write permission on the directory being moved, because the kernel updates its .. entry. The runner does not have that on a root-owned .qwen.root-orig and the preceding chmod -R u+w cannot grant it, so the individual mv fails with EACCES even though the workspace parent is writable. That is the case the whole-workspace quarantine catches. Comment only, no executable line changes.
d0b25cf to
4fc011d
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:反向审计——评审时间预算不足,未能开始第 1 轮。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.3)
What this PR does
Rewrites the comment above the quarantine fallback in the
review-prjob'sClean stale .qwen before checkoutstep. Comment only, zero executable lineschange.
Why it's needed
I opened this against #10392 by mistake. The sweep that issue asks for is
already on
main: #10214 (commit379bef96) added it to this workflow alongwith the
cleanIdx < checkoutIdxpin inscripts/tests/review-worktree-cleanup-workflow.test.js, even though itsdescription said it was scoped to
ci.yml. Nothing here changes behaviour, sothe
Fixeslink is gone and #10392 should be closed against #10214 instead.What's left is still worth keeping on its own. The old comment stated the
fallback but not the reason it exists:
Reading that, the fallback looks redundant, since the
mvright above italready targets a writable parent. The missing piece is that renaming a
directory across parents also needs write permission on the directory being
moved, because the kernel has to update its
..entry. For a root-owned.qwen.root-origthe runner does not have that, and the precedingchmod -R u+wcannot grant it either, so the individualmvfails withEACCESeven though the workspace parent is perfectly writable. That is thecase the whole-workspace quarantine exists to catch, and it was not obvious
from the code.
Reviewer Test Plan
How to verify
Read the diff. It touches
#lines only. The comment-strippedrun:body isbyte-identical to
main:Evidence (Before & After)
N/A (comment-only change)
Tested on
Risk & Scope
Linked Issues
None. The actual fix for #10392 shipped in #10214.
中文说明
仅改写
review-pr任务中Clean stale .qwen before checkout步骤里 quarantine兜底逻辑上方的注释,无任何可执行行变化。
本 PR 最初错误地关联了 #10392。该 issue 要求的清理逻辑其实已经在
main上(#10214,commit
379bef96),因此已移除Fixes关联,#10392 应改为指向#10214 关闭。
保留注释改写的理由:旧注释只说明了兜底行为,没有说明它为何存在。关键点在于,
跨父目录重命名目录还需要对被移动目录本身有写权限(内核需更新其
..项)。对 root 所有的
.qwen.root-orig,runner 不具备该权限,前面的chmod -R u+w也无法赋予,因此即使工作区父目录可写,单独的
mv仍会以EACCES失败。这正是整体工作区 quarantine 所要处理的场景,而这一点从代码中并不显而易见。