-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(review): repair permissions before giving up on worktree cleanup #9748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
afeaedf
04ace8a
be0f808
7a9fe5d
dcea310
490bafe
5b8f50d
d6c6db6
8fd0342
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1842,6 +1842,20 @@ jobs: | |||||||||||||
| # the next job on this reused runner can delete qwen-review/* branches. | ||||||||||||||
| # The sweep deletes all review artifacts, not just this PR's: safe because | ||||||||||||||
| # a runner executes one job at a time. | ||||||||||||||
| # | ||||||||||||||
| # The removal owns its own permission repair. A containerised job on this | ||||||||||||||
| # shared pool can leave a review worktree owned by another uid and | ||||||||||||||
| # read-only (measured, run 32577821716 / PR #9718: a leftover | ||||||||||||||
| # scratch-verify tree held files this job's user could not unlink, and | ||||||||||||||
| # the NEXT review's checkout died on them with EACCES — both the | ||||||||||||||
| # pre-checkout ownership restore and the checkout's own wipe degraded | ||||||||||||||
| # because the runner had no passwordless sudo). A removal that gives up | ||||||||||||||
| # on the first EACCES re-poisons the next job, so a failed rm gets a | ||||||||||||||
| # repair ladder instead: chmod what this user owns, then passwordless | ||||||||||||||
| # sudo chown/chmod where the pool member has it, each followed by a | ||||||||||||||
| # retry. Members without sudo still degrade to a named warning — | ||||||||||||||
| # nothing unprivileged can remove a foreign-owned tree — but the heal | ||||||||||||||
| # chain must never fail the job. | ||||||||||||||
| - name: 'Clean review worktrees' | ||||||||||||||
| if: 'always()' | ||||||||||||||
| timeout-minutes: 5 | ||||||||||||||
|
|
@@ -1853,6 +1867,67 @@ jobs: | |||||||||||||
| fi | ||||||||||||||
|
|
||||||||||||||
| GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE") | ||||||||||||||
|
|
||||||||||||||
| # The repair ladder for one leftover tree (see the step comment). | ||||||||||||||
| # A path outside the workspace or resolving through symlinks is | ||||||||||||||
| # refused rather than repaired: the sudo leg escalates to root, | ||||||||||||||
| # and a planted link would aim a chown/chmod -R outside the | ||||||||||||||
| # workspace. Warning echoes strip CR and LF from every path | ||||||||||||||
| # expansion first: leftover names are untrusted glob entries, and a | ||||||||||||||
| # fresh line on the runner's stdout — which it splits on bare CR as | ||||||||||||||
| # well as LF — would parse as a workflow command. | ||||||||||||||
| remove_review_tree() { | ||||||||||||||
| local abs="$1" | ||||||||||||||
| case "$abs" in | ||||||||||||||
| /*) : ;; | ||||||||||||||
| *) abs="$GITHUB_WORKSPACE/$abs" ;; | ||||||||||||||
| esac | ||||||||||||||
| [ -e "$abs" ] || [ -L "$abs" ] || return 0 | ||||||||||||||
| rm -rf "$abs" 2>/dev/null && return 0 | ||||||||||||||
| # Refuse a path that resolves through symlinks, but compare | ||||||||||||||
| # against the workspace's OWN resolved path: an ancestor the | ||||||||||||||
| # workspace itself sits under (a macOS /tmp -> /private/tmp | ||||||||||||||
| # local run) is legitimate and must not read as a redirect — | ||||||||||||||
| # only a symlink planted BELOW the workspace does. The refusal | ||||||||||||||
| # names the branch that fired so the on-call knows which case | ||||||||||||||
| # hit. | ||||||||||||||
| local ws_real rel abs_real reason='' | ||||||||||||||
| ws_real="$(realpath -- "$GITHUB_WORKSPACE" 2>/dev/null)" || | ||||||||||||||
| ws_real="$GITHUB_WORKSPACE" | ||||||||||||||
| case "$abs" in | ||||||||||||||
| "$GITHUB_WORKSPACE"/*) rel="${abs#"$GITHUB_WORKSPACE/"}" ;; | ||||||||||||||
| *) rel='' ;; | ||||||||||||||
| esac | ||||||||||||||
| abs_real="$(realpath -- "$abs" 2>/dev/null)" || abs_real='' | ||||||||||||||
| if [ -z "$rel" ]; then | ||||||||||||||
| reason='outside the workspace' | ||||||||||||||
| elif [ -L "$abs" ]; then | ||||||||||||||
| reason='path is a symlink' | ||||||||||||||
| elif [ -z "$abs_real" ]; then | ||||||||||||||
| reason='path could not be resolved' | ||||||||||||||
| elif [ "$abs_real" != "$ws_real/$rel" ]; then | ||||||||||||||
| reason='resolves through symlinks' | ||||||||||||||
| fi | ||||||||||||||
| if [ -n "$reason" ]; then | ||||||||||||||
| echo "::warning::refusing to repair review worktree path (${reason}): ${abs//[$'\r\n']/ }" | ||||||||||||||
| return 0 | ||||||||||||||
| fi | ||||||||||||||
| chmod -R u+rwX "$abs" 2>/dev/null || true | ||||||||||||||
| rm -rf "$abs" 2>/dev/null && return 0 | ||||||||||||||
| local sudo_probe='password-gated' | ||||||||||||||
| command -v sudo >/dev/null 2>&1 || sudo_probe='absent' | ||||||||||||||
| if command -v sudo >/dev/null 2>&1 && sudo -n true 2>/dev/null; then | ||||||||||||||
| sudo_probe='ok' | ||||||||||||||
| sudo -n chown -R "$(id -u):$(id -g)" "$abs" 2>/dev/null || true | ||||||||||||||
| sudo -n chmod -R u+rwX "$abs" 2>/dev/null || true | ||||||||||||||
| fi | ||||||||||||||
| rm -rf "$abs" 2>/dev/null && return 0 | ||||||||||||||
| echo "::warning::could not remove review worktree: ${abs//[$'\r\n']/ } (permission repair failed; sudo: $sudo_probe; owner: $(ls -ld "$abs" 2>/dev/null | awk 'NR==1 {print $3}'))" | ||||||||||||||
| # return 0 even when the warning echo fails: the heal chain must | ||||||||||||||
| # never fail the job. | ||||||||||||||
| return 0 | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+1924
to
+1929
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The new leftover loop is the first place raw shell-glob names reach step stdout. A leftover entry whose name embeds a newline followed by
Suggested change
中文说明新增的残留目录循环是 shell glob 原始文件名第一次直接进入步骤 stdout。名字中嵌入换行加 — qwen3.8-max via Qwen Code /review (v0.22.0) |
||||||||||||||
|
|
||||||||||||||
| "${GIT_SAFE[@]}" worktree prune -v || true | ||||||||||||||
| "${GIT_SAFE[@]}" worktree list --porcelain \ | ||||||||||||||
| | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \ | ||||||||||||||
|
|
@@ -1861,22 +1936,34 @@ jobs: | |||||||||||||
| # Registered paths come from leftover git metadata and are | ||||||||||||||
| # untrusted: the awk filter above matched by substring, so reject | ||||||||||||||
| # `..` traversal and re-anchor to the review prefix before the | ||||||||||||||
| # destructive remove. | ||||||||||||||
| # destructive remove. The skip warnings strip CR/LF from the | ||||||||||||||
| # path for the same reason the ladder's warnings do (above). | ||||||||||||||
| case "$worktree" in | ||||||||||||||
| */../*|../*|*/..) | ||||||||||||||
| echo "::warning::skipping suspicious review worktree path: $worktree" | ||||||||||||||
| echo "::warning::skipping suspicious review worktree path: ${worktree//[$'\r\n']/ }" | ||||||||||||||
| continue | ||||||||||||||
| ;; | ||||||||||||||
| "$GITHUB_WORKSPACE/.qwen/tmp/review-pr-"*) : ;; | ||||||||||||||
| *) | ||||||||||||||
| echo "::warning::skipping unexpected review worktree path: $worktree" | ||||||||||||||
| echo "::warning::skipping unexpected review worktree path: ${worktree//[$'\r\n']/ }" | ||||||||||||||
| continue | ||||||||||||||
| ;; | ||||||||||||||
| esac | ||||||||||||||
| # `git worktree remove` unlinks entries the same way rm does, | ||||||||||||||
| # so a foreign-owned entry defeats it too; the repair ladder | ||||||||||||||
| # retries it, and whatever git still leaves behind goes through | ||||||||||||||
| # the same ladder below (registrations are pruned afterwards). | ||||||||||||||
| "${GIT_SAFE[@]}" worktree remove --force "$worktree" || | ||||||||||||||
| echo "::warning::could not remove review worktree: $worktree" | ||||||||||||||
| remove_review_tree "$worktree" | ||||||||||||||
| done || true | ||||||||||||||
| rm -rf .qwen/tmp/review-pr-* 2>/dev/null || true | ||||||||||||||
| # Survivors of the glob are exactly the permission-poisoned trees; | ||||||||||||||
| # run each through the repair ladder individually so one poisoned | ||||||||||||||
| # entry cannot mask its siblings. | ||||||||||||||
| for leftover in .qwen/tmp/review-pr-*; do | ||||||||||||||
| [ -e "$leftover" ] || [ -L "$leftover" ] || continue | ||||||||||||||
| remove_review_tree "$leftover" | ||||||||||||||
| done | ||||||||||||||
| "${GIT_SAFE[@]}" worktree prune -v || true | ||||||||||||||
| "${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \ | ||||||||||||||
| | while read -r review_ref; do | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The comment says each rung is "each followed by a retry", but
remove_review_treeretriesrm -rfexactly once, after ALL rungs — there is no retry between the chmod rung and the sudo rung, so the root leg escalates even when chmod already repaired the tree. This file is deliberately comment-driven and its cleanup recipe is contract-pinned by tests, so a maintainer auditing when the root leg fires will conclude a chmod-repaired tree is removed before sudo is attempted — the opposite of what runs. Reword the comment (or insertrm -rf "$abs" 2>/dev/null && return 0between the two rungs to make the code match, which also skips the root leg when chmod sufficed):中文说明
注释称每个梯级都“各跟一次重试”,但
remove_review_tree只在所有梯级之后重试一次rm -rf——chmod 梯级与 sudo 梯级之间并没有重试,因此即使 chmod 已经修好了树,root 梯级仍会升级执行。该文件刻意以注释驱动,且其清理流程被契约测试 pin 住,审计 root 梯级触发时机的维护者会得出“chmod 修好的树会在动用 sudo 之前被删除”的结论——与实际行为相反。建议改写注释(或在两个梯级之间插入rm -rf "$abs" 2>/dev/null && return 0使代码与注释一致,这样 chmod 已修复时还能跳过 root 梯级)(见上方 suggestion)。— qwen3.8-max via Qwen Code /review (v0.22.0)