Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/.size-baseline
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
5942 qwen-autofix-fork-signal.yml
397656 qwen-autofix.yml
7061 qwen-ci-flaky-rerun.yml
151937 qwen-code-pr-review.yml
158010 qwen-code-pr-review.yml
79041 qwen-fleet-shepherd.yml
20525 qwen-issue-followup-bot.yml
5760 qwen-pr-safety-precheck.yml
Expand Down
88 changes: 87 additions & 1 deletion .github/workflows/qwen-code-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 —
Comment on lines +1854 to +1856

Copy link
Copy Markdown
Collaborator

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_tree retries rm -rf exactly 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 insert rm -rf "$abs" 2>/dev/null && return 0 between the two rungs to make the code match, which also skips the root leg when chmod sufficed):

Suggested change
# 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 —
# repair ladder instead: chmod what this user owns, then passwordless
# sudo chown/chmod where the pool member has it, with a single retry
# after the ladder. Members without sudo still degrade to a named warning —
中文说明

注释称每个梯级都“各跟一次重试”,但 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)

# 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
Expand All @@ -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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 ::command is echoed here un-sanitized, and the Actions runner parses ::-prefixed lines as workflow commands. A prior containerised job on this pool — exactly the actor this step exists for — can leave a name like review-pr-x$'\n::stop-commands::tok'; when the repair fails on a sudo-less member, this warning emits the raw name and the runner parses the embedded line. The reachable impact is bounded (set-env/add-path have been disabled since 2020, so this is annotation spoofing / stop-commands suppression rather than code execution), but the surface is new in this diff: pre-change, the glob rm -rf … || true never echoed leftover names, and the worktree-list loop's values come from git porcelain, which C-escapes newlines. Strip newlines before echoing:

Suggested change
rm -rf "$abs" 2>/dev/null && return 0
echo "::warning::could not remove review worktree: $abs (permission repair failed)"
}
rm -rf "$abs" 2>/dev/null && return 0
echo "::warning::could not remove review worktree: ${abs//$'\n'/ } (permission repair failed)"
}
中文说明

新增的残留目录循环是 shell glob 原始文件名第一次直接进入步骤 stdout。名字中嵌入换行加 ::command 的残留条目在这里会未经净化地被 echo 出来,而 Actions runner 会把以 :: 开头的行解析为 workflow 命令。池上先前的容器化 job(正是本步骤要处理的角色)可以留下形如 review-pr-x$'\n::stop-commands::tok' 的名字;当修复在无 sudo 成员上失败时,这条 warning 会原样输出该名字,runner 随即解析其中嵌入的命令行。可达影响有上限(set-env/add-path 自 2020 年已禁用,因此只能伪造 annotation / 触发 stop-commands 抑制,而非代码执行),但该注入面是本 diff 新引入的:改动前 glob 的 rm -rf … || true 从不 echo 残留名字,而 worktree 列表循环的值来自 git porcelain(会对换行做 C 转义)。建议在 echo 前剥离换行(见上方 suggestion)。

— 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 }' \
Expand All @@ -1873,10 +1948,21 @@ jobs:
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
Expand Down
Loading
Loading