Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
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
157 changes: 157 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,51 @@ jobs:
chmod -R u+w "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || true
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || echo "::warning::leaked .qwen; runner needs manual cleanup"
fi
# Interrupted reviews leave worktree registrations under .qwen/tmp/
# and qwen-review/* branches behind. prune drops registrations whose
# directories the rm above removed; worktree remove --force then
# clears any still-registered leftover directory (--force tolerates
# dirty contents), since a branch checked out in a live worktree
# cannot be deleted. If removal still fails, the registration
# survives and the branch delete below warns. The sweep deletes all
# review artifacts, not just the current PR's: safe because a runner
# executes one job at a time. Kept inline rather than a shared
# script: this runs pre-checkout on shared runners, where leftover
# workspace files are untrusted.
if [ -e "$GITHUB_WORKSPACE/.git" ]; then
GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" worktree list --porcelain \
| awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \
| while read -r worktree; do
[ -n "$worktree" ] || continue
# 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.
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
;;
esac
"${GIT_SAFE[@]}" worktree remove --force "$worktree" ||
echo "::warning::could not remove review worktree: $worktree"
done || true
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \
| while read -r stale_ref; do
if [ -n "$stale_ref" ]; then
"${GIT_SAFE[@]}" branch -D "$stale_ref" ||
echo "::warning::could not remove review branch: $stale_ref"
fi
done || true
fi

# On PRs, check out refs/pull/N/head (the immutable PR head, published the
# instant the branch is pushed) instead of github.ref. github.ref is the
Expand Down Expand Up @@ -523,6 +568,62 @@ jobs:
fi
chmod -R u+rwX "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chmod -R u+rwX "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace write permissions; checkout may fail on leftover read-only files"

# 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.
- 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
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || echo "::warning::leaked .qwen; runner needs manual cleanup"
fi
# Interrupted reviews leave worktree registrations under .qwen/tmp/
# and qwen-review/* branches behind. prune drops registrations whose
# directories the rm above removed; worktree remove --force then
# clears any still-registered leftover directory (--force tolerates
# dirty contents), since a branch checked out in a live worktree
# cannot be deleted. If removal still fails, the registration
# survives and the branch delete below warns. The sweep deletes all
# review artifacts, not just the current PR's: safe because a runner
# executes one job at a time. Kept inline rather than a shared
# script: this runs pre-checkout on shared runners, where leftover
# workspace files are untrusted.
if [ -e "$GITHUB_WORKSPACE/.git" ]; then
GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" worktree list --porcelain \
| awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \
| while read -r worktree; do
[ -n "$worktree" ] || continue
# 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.
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
;;
esac
"${GIT_SAFE[@]}" worktree remove --force "$worktree" ||
echo "::warning::could not remove review worktree: $worktree"
done || true
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \
| while read -r stale_ref; do
if [ -n "$stale_ref" ]; then
"${GIT_SAFE[@]}" branch -D "$stale_ref" ||
echo "::warning::could not remove review branch: $stale_ref"
fi
done || true
fi

- name: 'Checkout'
uses: 'actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd' # v6.0.2
with:
Expand Down Expand Up @@ -815,6 +916,62 @@ jobs:
fi
chmod -R u+rwX "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chmod -R u+rwX "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace write permissions; checkout may fail on leftover read-only files"

# 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.
- 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
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || echo "::warning::leaked .qwen; runner needs manual cleanup"
fi
# Interrupted reviews leave worktree registrations under .qwen/tmp/
# and qwen-review/* branches behind. prune drops registrations whose
# directories the rm above removed; worktree remove --force then
# clears any still-registered leftover directory (--force tolerates
# dirty contents), since a branch checked out in a live worktree
# cannot be deleted. If removal still fails, the registration
# survives and the branch delete below warns. The sweep deletes all
# review artifacts, not just the current PR's: safe because a runner
# executes one job at a time. Kept inline rather than a shared
# script: this runs pre-checkout on shared runners, where leftover
# workspace files are untrusted.
if [ -e "$GITHUB_WORKSPACE/.git" ]; then
GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" worktree list --porcelain \
| awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \
| while read -r worktree; do
[ -n "$worktree" ] || continue
# 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.
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
;;
esac
"${GIT_SAFE[@]}" worktree remove --force "$worktree" ||
echo "::warning::could not remove review worktree: $worktree"
done || true
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \
| while read -r stale_ref; do
if [ -n "$stale_ref" ]; then
"${GIT_SAFE[@]}" branch -D "$stale_ref" ||
echo "::warning::could not remove review branch: $stale_ref"
fi
done || true
fi

- name: 'Checkout'
uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3
with:
Expand Down
62 changes: 57 additions & 5 deletions .github/workflows/qwen-code-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -397,15 +397,17 @@ jobs:
echo "no prior workspace; nothing to clean"
exit 0
fi
GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")
rm -rf .qwen/tmp/review-pr-* 2>/dev/null || true
git worktree prune -v || true
git for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \
| while read -r stale_ref; do
if [ -n "$stale_ref" ]; then
git branch -D "$stale_ref" || true
"${GIT_SAFE[@]}" branch -D "$stale_ref" ||
echo "::warning::could not remove review branch: $stale_ref"
fi
done
git worktree prune -v || true
done || true
"${GIT_SAFE[@]}" worktree prune -v || true
echo "stale agent state cleaned"

# SECURITY: checkout trusted base code; /review fetches PR diff context.
Expand Down Expand Up @@ -989,6 +991,56 @@ jobs:
--repo "$GITHUB_REPOSITORY" \
--body "$body"

# A cancelled or timed-out review may not reach the CLI's process cleanup.
# Remove both the worktree directories and Git's worktree registrations so
# 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.
- name: 'Clean review worktrees'
if: 'always()'
timeout-minutes: 5
run: |-
set -uo pipefail
if [ ! -e .git ]; then
echo "no Git checkout; nothing to clean"
exit 0
fi

GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" worktree list --porcelain \
| awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \
| while read -r worktree; do
Comment on lines +1011 to +1013

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] 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
    ;;
esac

Verified 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)

[ -n "$worktree" ] || continue
# 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.
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
;;
esac
"${GIT_SAFE[@]}" worktree remove --force "$worktree" ||
echo "::warning::could not remove review worktree: $worktree"
done || true
rm -rf .qwen/tmp/review-pr-* 2>/dev/null || true
"${GIT_SAFE[@]}" worktree prune -v || true
"${GIT_SAFE[@]}" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \
| while read -r review_ref; do
[ -n "$review_ref" ] || continue
Comment on lines +1035 to +1037

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] 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"
fi

Verified 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)

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.

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 恢复逻辑,把它与守卫组合起来后我们很乐意再议。

"${GIT_SAFE[@]}" branch -D "$review_ref" ||
echo "::warning::could not remove review branch: $review_ref"
done || true
rm -f .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true
echo "review worktrees cleaned"

resolve-pr:
needs: ['authorize']
if: |-
Expand Down
8 changes: 8 additions & 0 deletions packages/cli/src/commands/review/lib/paths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ export const REVIEW_TMP_DIR = join('.qwen', 'tmp');
export const REVIEWS_DIR = join('.qwen', 'reviews');
export const REVIEW_CACHE_DIR = join('.qwen', 'review-cache');

/**
* Filename prefix for review-worktree lease files under `REVIEW_TMP_DIR`.
* Lives here, not in `review-worktree-lease.ts`, because the review
* workflow's cleanup sweep deletes leases by glob — the sweep pattern and
* the lease writer must share one definition (the cleanup spec pins both).
*/
export const LEASE_PREFIX = 'qwen-review-lease-';

/**
* Where the skill tees `qwen review parse-args`'s verdict (SKILL Step 0). A fixed,
* conventional name so a capture command can read back the effort the parser
Expand Down
7 changes: 5 additions & 2 deletions packages/cli/src/services/review-worktree-lease.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ import {
} from 'node:fs';
import { basename, isAbsolute, join, relative, resolve } from 'node:path';
import { createDebugLogger } from '@qwen-code/qwen-code-core';
import { REVIEW_TMP_DIR, reviewBranch } from '../commands/review/lib/paths.js';
import {
LEASE_PREFIX,
REVIEW_TMP_DIR,
reviewBranch,
} from '../commands/review/lib/paths.js';

const LEASE_PREFIX = 'qwen-review-lease-';
const GIT_TIMEOUT_MS = 120_000;
const debugLogger = createDebugLogger('REVIEW_WORKTREE_LEASE');

Expand Down
Loading
Loading