From 41d4c66bb90d929d9914af29cf0e7bd4adc70687 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 4 Aug 2026 00:09:18 +0800 Subject: [PATCH 01/10] fix(ci): clean review worktrees after cancellation --- .github/workflows/ci.yml | 17 ++++++++++++ .github/workflows/qwen-code-pr-review.yml | 32 +++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2339be1f9f7..fb8288d19f7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,6 +172,23 @@ 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 + if [ -e "$GITHUB_WORKSPACE/.git" ]; then + git -C "$GITHUB_WORKSPACE" worktree prune -v || true + git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ + | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \ + | while read -r worktree; do + [ -n "$worktree" ] || continue + git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || true + done + git -C "$GITHUB_WORKSPACE" worktree prune -v || true + git -C "$GITHUB_WORKSPACE" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \ + | while read -r stale_ref; do + if [ -n "$stale_ref" ]; then + git -C "$GITHUB_WORKSPACE" branch -D "$stale_ref" || true + fi + done + git -C "$GITHUB_WORKSPACE" worktree prune -v || 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 diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index a24c5d5eaee..c9ce87e08a8 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -981,6 +981,38 @@ 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. + - 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 worktree prune -v || true + git worktree list --porcelain \ + | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \ + | while read -r worktree; do + [ -n "$worktree" ] || continue + git worktree remove --force "$worktree" || + echo "::warning::could not remove review worktree: $worktree" + done + git worktree prune -v || true + git for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \ + | while read -r review_ref; do + [ -n "$review_ref" ] || continue + git branch -D "$review_ref" || + echo "::warning::could not remove review branch: $review_ref" + done + git worktree prune -v || true + rm -f .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true + echo "review worktrees cleaned" + resolve-pr: needs: ['authorize'] if: |- From d1f976a5c20f18abf9662109af2f7fdc0fd139f1 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 4 Aug 2026 00:15:43 +0800 Subject: [PATCH 02/10] fix(ci): remove orphaned review worktree directories --- .github/workflows/ci.yml | 1 - .github/workflows/qwen-code-pr-review.yml | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb8288d19f7..dbd7ec037ee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -187,7 +187,6 @@ jobs: git -C "$GITHUB_WORKSPACE" branch -D "$stale_ref" || true fi done - git -C "$GITHUB_WORKSPACE" worktree prune -v || true fi # On PRs, check out refs/pull/N/head (the immutable PR head, published the diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index c9ce87e08a8..d63e4b5c404 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1002,6 +1002,7 @@ jobs: git worktree remove --force "$worktree" || echo "::warning::could not remove review worktree: $worktree" done + 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/*' \ | while read -r review_ref; do @@ -1009,7 +1010,6 @@ jobs: git branch -D "$review_ref" || echo "::warning::could not remove review branch: $review_ref" done - git worktree prune -v || true rm -f .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true echo "review worktrees cleaned" From 02909bec2f1cd4cdf58ef6ba04352125ef9f538a Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 3 Aug 2026 17:52:31 +0000 Subject: [PATCH 03/10] fix(tests): sync qwen-resolve-workflow expectations with externalized review timeouts (#8474) --- scripts/tests/qwen-resolve-workflow.test.js | 29 ++++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index 904f3fa66a1..a07558f62ea 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -346,7 +346,9 @@ describe('qwen resolve workflow', () => { const contextStep = step(reviewJob, 'Resolve PR context'); const runStep = step(reviewJob, 'Run review'); - expect(reviewJob).toContain('timeout-minutes: 300'); + expect(reviewJob).toContain( + "timeout-minutes: '${{ fromJSON(vars.QWEN_REVIEW_JOB_TIMEOUT_MINUTES) }}'", + ); expect(contextStep).toContain('DEFAULT_TIMEOUT_MINUTES=180'); expect(contextStep).toContain('case "$token" in'); expect(contextStep).toContain('--timeout=*)'); @@ -354,7 +356,12 @@ describe('qwen resolve workflow', () => { expect(contextStep).toContain('timeout=*)'); expect(contextStep).toContain('TIMEOUT_MINUTES="${token#timeout=}"'); expect(runStep).toContain('if [ "${#TIMEOUT_MINUTES}" -gt 3 ]; then'); - expect(runStep).toContain('timeout_minutes must not exceed 240 minutes'); + expect(runStep).toContain( + 'MAX_TIMEOUT_MINUTES="${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}"', + ); + expect(runStep).toContain( + 'timeout_minutes must not exceed ${MAX_TIMEOUT_MINUTES} minutes', + ); expect(runStep).toContain('QWEN_TIMEOUT="$EFFECTIVE_TIMEOUT_MINUTES"'); expect(runStep).not.toContain('QWEN_TIMEOUT=$((TIMEOUT_MINUTES - 5))'); }); @@ -372,8 +379,9 @@ describe('qwen resolve workflow', () => { ); // Auto-tiering only applies without an explicit --timeout, keys off - // additions + deletions, and never exceeds the 240 cap: small PRs keep 180, - // anything larger gets the full 240. + // additions + deletions, and never exceeds the + // QWEN_REVIEW_MAX_TIMEOUT_MINUTES cap: small PRs keep 180, anything + // larger gets the full cap. expect(runStep).toContain('EFFECTIVE_TIMEOUT_MINUTES="$TIMEOUT_MINUTES"'); expect(runStep).toContain( 'if [ "${TIMEOUT_EXPLICIT:-false}" != "true" ]; then', @@ -381,7 +389,9 @@ describe('qwen resolve workflow', () => { expect(runStep).toContain('--json additions,deletions'); expect(runStep).toContain('if [ "$PR_SIZE_LINES" -le 300 ]; then'); expect(runStep).toContain('EFFECTIVE_TIMEOUT_MINUTES=180'); - expect(runStep).toContain('EFFECTIVE_TIMEOUT_MINUTES=240'); + expect(runStep).toContain( + 'EFFECTIVE_TIMEOUT_MINUTES="${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}"', + ); expect(runStep).not.toContain('EFFECTIVE_TIMEOUT_MINUTES=210'); expect(runStep).toContain( 'echo "effective_timeout_minutes=$EFFECTIVE_TIMEOUT_MINUTES"', @@ -403,9 +413,14 @@ describe('qwen resolve workflow', () => { expect(fallbackStep).toContain( "TIMEOUT_MINUTES: '${{ steps.review.outputs.effective_timeout_minutes || steps.context.outputs.timeout_minutes }}'", ); - expect(fallbackStep).toContain('@qwen-code /review --timeout=240'); expect(fallbackStep).toContain( - 'This run already used the maximum 240 minute timeout.', + 'MAX_TIMEOUT_MINUTES="${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}"', + ); + expect(fallbackStep).toContain( + '@qwen-code /review --timeout=${MAX_TIMEOUT_MINUTES}', + ); + expect(fallbackStep).toContain( + 'This run already used the maximum ${MAX_TIMEOUT_MINUTES} minute timeout.', ); expect(fallbackStep).toContain('**Qwen Code review timed out.**'); expect(fallbackStep).not.toContain( From 0a8d999db53c80ba35cd3414cce69ab9fe8b3564 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Mon, 3 Aug 2026 19:18:06 +0000 Subject: [PATCH 04/10] fix(ci): pin review worktree cleanup patterns to paths.ts (#8474) --- .github/workflows/ci.yml | 11 ++- .../review-worktree-cleanup-workflow.test.js | 94 +++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 scripts/tests/review-worktree-cleanup-workflow.test.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dbd7ec037ee..9c7f13bdfc0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,19 +172,26 @@ 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; the rm above removed the + # directories, so sync Git's bookkeeping and drop the branches. + # 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 -C "$GITHUB_WORKSPACE" worktree prune -v || true git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \ | while read -r worktree; do [ -n "$worktree" ] || continue - git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || true + git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || + echo "::warning::could not remove review worktree: $worktree" done git -C "$GITHUB_WORKSPACE" worktree prune -v || true git -C "$GITHUB_WORKSPACE" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \ | while read -r stale_ref; do if [ -n "$stale_ref" ]; then - git -C "$GITHUB_WORKSPACE" branch -D "$stale_ref" || true + git -C "$GITHUB_WORKSPACE" branch -D "$stale_ref" || + echo "::warning::could not remove review branch: $stale_ref" fi done fi diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js new file mode 100644 index 00000000000..ff77f91c375 --- /dev/null +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -0,0 +1,94 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { spawnSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import { describe, expect, it } from 'vitest'; +import { parse } from 'yaml'; +import { + reviewBranch, + worktreePath, +} from '../../packages/cli/src/commands/review/lib/paths.js'; + +// The cleanup steps in ci.yml and qwen-code-pr-review.yml hard-code the +// review-artifact layout owned by worktreePath()/reviewBranch() in paths.ts. +// Derive the expected patterns from that module so renaming the layout there +// fails the build here instead of silently no-op-ing the sweeps on the shared +// runners — a suffix rename already broke a sweeper once (see paths.ts). +const probePr = 12345; +const toPosix = (value) => value.replace(/\\/g, '/'); +const worktreePrefix = toPosix(worktreePath(probePr)).slice( + 0, + -`${probePr}`.length, +); +const branchFamily = toPosix(reviewBranch(probePr)).slice( + 0, + -`pr-${probePr}`.length, +); + +const ciCleanStep = parse( + readFileSync('.github/workflows/ci.yml', 'utf8'), +).jobs.test.steps.find( + (s) => s.name === 'Clean stale .qwen before checkout', +).run; +const reviewCleanStep = parse( + readFileSync('.github/workflows/qwen-code-pr-review.yml', 'utf8'), +).jobs['review-pr'].steps.find((s) => s.name === 'Clean review worktrees').run; + +// prune (sync registrations) -> force-remove -> prune (drop now-stale +// entries) -> delete branches: a branch checked out in a live worktree +// cannot be deleted, so worktree removal must precede the branch sweep. +function expectCleanupRecipe(run) { + expect(run).toContain(`index($0, "/${worktreePrefix}")`); + expect(run).toContain('worktree remove --force'); + expect(run).toContain(`refs/heads/${branchFamily}*`); + const remove = run.indexOf('worktree remove --force'); + const firstPrune = run.indexOf('worktree prune'); + expect(firstPrune).toBeGreaterThan(-1); + expect(firstPrune).toBeLessThan(remove); + expect(run.indexOf('worktree prune', remove)).toBeGreaterThan(remove); + expect(run.indexOf(`refs/heads/${branchFamily}*`)).toBeGreaterThan(remove); +} + +const awkAvailable = spawnSync('awk', ['BEGIN { exit 0 }']).status === 0; + +describe('review worktree cleanup steps', () => { + it('keeps the ci.yml pre-checkout sweep pinned to paths.ts', () => { + expectCleanupRecipe(ciCleanStep); + }); + + it('keeps the review-job cleanup sweep pinned to paths.ts', () => { + expectCleanupRecipe(reviewCleanStep); + // Fallback for worktree directories Git no longer knows about. + expect(reviewCleanStep).toContain(`rm -rf ${worktreePrefix}*`); + }); + + it('uses one identical worktree filter at both sites', () => { + const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1]; + expect(filter).toBeTruthy(); + expect(ciCleanStep).toContain(`awk '${filter}'`); + }); + + it.skipIf(!awkAvailable)( + 'filter selects review worktrees only, never the main checkout', + () => { + const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1]; + const main = '/home/runner/work/qwen-code/qwen-code'; + const review = `${main}/.qwen/tmp/review-pr-42`; + const out = spawnSync('awk', [filter], { + input: [ + `worktree ${main}`, + `worktree ${review}`, + 'branch qwen-review/pr-42', + '', + ].join('\n'), + encoding: 'utf8', + }); + expect(out.status).toBe(0); + expect(out.stdout.trim()).toBe(review); + }, + ); +}); From eb1f6a6a95063ea097b7e5a8e9fe01519819cf12 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 4 Aug 2026 11:58:43 +0000 Subject: [PATCH 05/10] fix(ci): harden review cleanup sweeps and cover integration_cli (#8474) --- .github/workflows/ci.yml | 38 ++++++++++++++++++- .github/workflows/qwen-code-pr-review.yml | 6 +-- .../review-worktree-cleanup-workflow.test.js | 33 ++++++++++++---- 3 files changed, 65 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d60d49a94db..874712db0fe 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -194,7 +194,7 @@ jobs: [ -n "$worktree" ] || continue git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || echo "::warning::could not remove review worktree: $worktree" - done + done || true git -C "$GITHUB_WORKSPACE" worktree prune -v || true git -C "$GITHUB_WORKSPACE" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \ | while read -r stale_ref; do @@ -202,7 +202,7 @@ jobs: git -C "$GITHUB_WORKSPACE" branch -D "$stale_ref" || echo "::warning::could not remove review branch: $stale_ref" fi - done + done || true fi # On PRs, check out refs/pull/N/head (the immutable PR head, published the @@ -838,6 +838,40 @@ 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; the rm above removed the + # directories, so sync Git's bookkeeping and drop the branches. + # 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 -C "$GITHUB_WORKSPACE" worktree prune -v || true + git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ + | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \ + | while read -r worktree; do + [ -n "$worktree" ] || continue + git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || + echo "::warning::could not remove review worktree: $worktree" + done || true + git -C "$GITHUB_WORKSPACE" worktree prune -v || true + git -C "$GITHUB_WORKSPACE" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \ + | while read -r stale_ref; do + if [ -n "$stale_ref" ]; then + git -C "$GITHUB_WORKSPACE" 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: diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index c5233537d29..777ecd06a18 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -404,7 +404,7 @@ jobs: if [ -n "$stale_ref" ]; then git branch -D "$stale_ref" || true fi - done + done || true git worktree prune -v || true echo "stale agent state cleaned" @@ -1009,7 +1009,7 @@ jobs: [ -n "$worktree" ] || continue git worktree remove --force "$worktree" || echo "::warning::could not remove review worktree: $worktree" - done + done || true 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/*' \ @@ -1017,7 +1017,7 @@ jobs: [ -n "$review_ref" ] || continue git branch -D "$review_ref" || echo "::warning::could not remove review branch: $review_ref" - done + done || true rm -f .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true echo "review worktrees cleaned" diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index ff77f91c375..7a088ced256 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -29,14 +29,22 @@ const branchFamily = toPosix(reviewBranch(probePr)).slice( -`pr-${probePr}`.length, ); -const ciCleanStep = parse( - readFileSync('.github/workflows/ci.yml', 'utf8'), -).jobs.test.steps.find( +const ciYaml = parse(readFileSync('.github/workflows/ci.yml', 'utf8')); +const ciCleanStep = ciYaml.jobs.test.steps.find( (s) => s.name === 'Clean stale .qwen before checkout', ).run; -const reviewCleanStep = parse( +const integrationCleanStep = ciYaml.jobs.integration_cli.steps.find( + (s) => s.name === 'Clean stale .qwen before checkout', +).run; +const reviewYaml = parse( readFileSync('.github/workflows/qwen-code-pr-review.yml', 'utf8'), -).jobs['review-pr'].steps.find((s) => s.name === 'Clean review worktrees').run; +); +const reviewCleanStep = reviewYaml.jobs['review-pr'].steps.find( + (s) => s.name === 'Clean review worktrees', +).run; +const agentStateCleanStep = reviewYaml.jobs['review-pr'].steps.find( + (s) => s.name === 'Clean stale agent state', +).run; // prune (sync registrations) -> force-remove -> prune (drop now-stale // entries) -> delete branches: a branch checked out in a live worktree @@ -56,20 +64,31 @@ function expectCleanupRecipe(run) { const awkAvailable = spawnSync('awk', ['BEGIN { exit 0 }']).status === 0; describe('review worktree cleanup steps', () => { - it('keeps the ci.yml pre-checkout sweep pinned to paths.ts', () => { + it('keeps the ci.yml test-job sweep pinned to paths.ts', () => { expectCleanupRecipe(ciCleanStep); }); + it('keeps the ci.yml integration_cli sweep pinned to paths.ts', () => { + expectCleanupRecipe(integrationCleanStep); + }); + it('keeps the review-job cleanup sweep pinned to paths.ts', () => { expectCleanupRecipe(reviewCleanStep); // Fallback for worktree directories Git no longer knows about. expect(reviewCleanStep).toContain(`rm -rf ${worktreePrefix}*`); }); - it('uses one identical worktree filter at both sites', () => { + it('keeps the pre-checkout agent-state sweep pinned to paths.ts', () => { + // Directories are rm -rf'd first there, so no `worktree remove` to pin. + expect(agentStateCleanStep).toContain(`rm -rf ${worktreePrefix}*`); + expect(agentStateCleanStep).toContain(`refs/heads/${branchFamily}*`); + }); + + it('uses one identical worktree filter at every list-driven sweep', () => { const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1]; expect(filter).toBeTruthy(); expect(ciCleanStep).toContain(`awk '${filter}'`); + expect(integrationCleanStep).toContain(`awk '${filter}'`); }); it.skipIf(!awkAvailable)( From f7f09548310650a21d5f2ee4e33ef85b3afa820e Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 4 Aug 2026 14:05:56 +0000 Subject: [PATCH 06/10] fix(ci): extend review cleanup sweep to web_shell_e2e_smoke (#8474) --- .github/workflows/ci.yml | 59 ++++++++++++-- packages/cli/src/commands/review/lib/paths.ts | 8 ++ .../cli/src/services/review-worktree-lease.ts | 7 +- .../review-worktree-cleanup-workflow.test.js | 76 +++++++++++++------ 4 files changed, 118 insertions(+), 32 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 874712db0fe..cb1d581653d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -182,10 +182,13 @@ jobs: 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; the rm above removed the - # directories, so sync Git's bookkeeping and drop the branches. - # Kept inline rather than a shared script: this runs pre-checkout on - # shared runners, where leftover workspace files are untrusted. + # and qwen-review/* branches behind. prune drops registrations for + # directories the rm above removed; worktree remove --force then + # clears the directories that survived the rm (root-owned or + # read-only leftovers), since a branch checked out in a live + # worktree cannot be deleted. 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 -C "$GITHUB_WORKSPACE" worktree prune -v || true git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ @@ -546,6 +549,43 @@ 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 for + # directories the rm above removed; worktree remove --force then + # clears the directories that survived the rm (root-owned or + # read-only leftovers), since a branch checked out in a live + # worktree cannot be deleted. 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 -C "$GITHUB_WORKSPACE" worktree prune -v || true + git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ + | awk '$1 == "worktree" && index($0, "/.qwen/tmp/review-pr-") > 0 { sub(/^worktree /, ""); print }' \ + | while read -r worktree; do + [ -n "$worktree" ] || continue + git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || + echo "::warning::could not remove review worktree: $worktree" + done || true + git -C "$GITHUB_WORKSPACE" worktree prune -v || true + git -C "$GITHUB_WORKSPACE" for-each-ref --format='%(refname:short)' 'refs/heads/qwen-review/*' \ + | while read -r stale_ref; do + if [ -n "$stale_ref" ]; then + git -C "$GITHUB_WORKSPACE" 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: @@ -849,10 +889,13 @@ jobs: 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; the rm above removed the - # directories, so sync Git's bookkeeping and drop the branches. - # Kept inline rather than a shared script: this runs pre-checkout on - # shared runners, where leftover workspace files are untrusted. + # and qwen-review/* branches behind. prune drops registrations for + # directories the rm above removed; worktree remove --force then + # clears the directories that survived the rm (root-owned or + # read-only leftovers), since a branch checked out in a live + # worktree cannot be deleted. 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 -C "$GITHUB_WORKSPACE" worktree prune -v || true git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ diff --git a/packages/cli/src/commands/review/lib/paths.ts b/packages/cli/src/commands/review/lib/paths.ts index 11e6014aa1a..93a266b1c54 100644 --- a/packages/cli/src/commands/review/lib/paths.ts +++ b/packages/cli/src/commands/review/lib/paths.ts @@ -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 diff --git a/packages/cli/src/services/review-worktree-lease.ts b/packages/cli/src/services/review-worktree-lease.ts index 31b9bbfe3b2..cef01c3c1b5 100644 --- a/packages/cli/src/services/review-worktree-lease.ts +++ b/packages/cli/src/services/review-worktree-lease.ts @@ -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'); diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index 7a088ced256..adc454f813a 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -9,15 +9,18 @@ import { readFileSync } from 'node:fs'; import { describe, expect, it } from 'vitest'; import { parse } from 'yaml'; import { + LEASE_PREFIX, + REVIEW_TMP_DIR, reviewBranch, worktreePath, } from '../../packages/cli/src/commands/review/lib/paths.js'; // The cleanup steps in ci.yml and qwen-code-pr-review.yml hard-code the -// review-artifact layout owned by worktreePath()/reviewBranch() in paths.ts. -// Derive the expected patterns from that module so renaming the layout there -// fails the build here instead of silently no-op-ing the sweeps on the shared -// runners — a suffix rename already broke a sweeper once (see paths.ts). +// review-artifact layout owned by paths.ts: worktreePath()/reviewBranch() +// and LEASE_PREFIX. Derive the expected patterns from that module so +// renaming the layout there fails the build here instead of silently +// no-op-ing the sweeps on the shared runners — a suffix rename already +// broke a sweeper once (see paths.ts). const probePr = 12345; const toPosix = (value) => value.replace(/\\/g, '/'); const worktreePrefix = toPosix(worktreePath(probePr)).slice( @@ -30,12 +33,20 @@ const branchFamily = toPosix(reviewBranch(probePr)).slice( ); const ciYaml = parse(readFileSync('.github/workflows/ci.yml', 'utf8')); -const ciCleanStep = ciYaml.jobs.test.steps.find( - (s) => s.name === 'Clean stale .qwen before checkout', -).run; -const integrationCleanStep = ciYaml.jobs.integration_cli.steps.find( - (s) => s.name === 'Clean stale .qwen before checkout', -).run; +// Every ci.yml job that checks out on the shared self-hosted pool inherits a +// possibly dirty workspace. Enumerate by pool + checkout instead of job name +// so the next such job fails here instead of on the runners. +const ciCleanSteps = Object.entries(ciYaml.jobs) + .filter( + ([, job]) => + JSON.stringify(job['runs-on']).includes('ubuntu_runner') && + job.steps.some((s) => String(s.uses ?? '').includes('actions/checkout')), + ) + .map(([id, job]) => ({ + id, + run: job.steps.find((s) => s.name === 'Clean stale .qwen before checkout') + ?.run, + })); const reviewYaml = parse( readFileSync('.github/workflows/qwen-code-pr-review.yml', 'utf8'), ); @@ -53,29 +64,49 @@ function expectCleanupRecipe(run) { expect(run).toContain(`index($0, "/${worktreePrefix}")`); expect(run).toContain('worktree remove --force'); expect(run).toContain(`refs/heads/${branchFamily}*`); - const remove = run.indexOf('worktree remove --force'); - const firstPrune = run.indexOf('worktree prune'); + // Order assertions below cover the commands only: comments may name the + // recipe pieces out of order when explaining them. + const code = run + .split('\n') + .filter((line) => !line.trim().startsWith('#')) + .join('\n'); + const remove = code.indexOf('worktree remove --force'); + const firstPrune = code.indexOf('worktree prune'); expect(firstPrune).toBeGreaterThan(-1); expect(firstPrune).toBeLessThan(remove); - expect(run.indexOf('worktree prune', remove)).toBeGreaterThan(remove); - expect(run.indexOf(`refs/heads/${branchFamily}*`)).toBeGreaterThan(remove); + expect(code.indexOf('worktree prune', remove)).toBeGreaterThan(remove); + expect(code.indexOf(`refs/heads/${branchFamily}*`)).toBeGreaterThan(remove); } const awkAvailable = spawnSync('awk', ['BEGIN { exit 0 }']).status === 0; describe('review worktree cleanup steps', () => { - it('keeps the ci.yml test-job sweep pinned to paths.ts', () => { - expectCleanupRecipe(ciCleanStep); - }); - - it('keeps the ci.yml integration_cli sweep pinned to paths.ts', () => { - expectCleanupRecipe(integrationCleanStep); + it('keeps every shared-pool ci.yml checkout sweep pinned to paths.ts', () => { + expect(ciCleanSteps.map(({ id }) => id)).toEqual( + expect.arrayContaining([ + 'test', + 'web_shell_e2e_smoke', + 'integration_cli', + ]), + ); + for (const { id, run } of ciCleanSteps) { + expect( + run, + `job "${id}" checks out on the shared pool and must clean stale .qwen state first`, + ).toBeDefined(); + expectCleanupRecipe(run); + } }); it('keeps the review-job cleanup sweep pinned to paths.ts', () => { expectCleanupRecipe(reviewCleanStep); // Fallback for worktree directories Git no longer knows about. expect(reviewCleanStep).toContain(`rm -rf ${worktreePrefix}*`); + // Leases are session+prompt scoped so a stale one is inert, but the glob + // must stay in sync with LEASE_PREFIX or it silently never matches. + expect(reviewCleanStep).toContain( + `rm -f ${toPosix(REVIEW_TMP_DIR)}/${LEASE_PREFIX}pr-*.json`, + ); }); it('keeps the pre-checkout agent-state sweep pinned to paths.ts', () => { @@ -87,8 +118,9 @@ describe('review worktree cleanup steps', () => { it('uses one identical worktree filter at every list-driven sweep', () => { const filter = reviewCleanStep.match(/awk '([^']+)'/)?.[1]; expect(filter).toBeTruthy(); - expect(ciCleanStep).toContain(`awk '${filter}'`); - expect(integrationCleanStep).toContain(`awk '${filter}'`); + for (const { id, run } of ciCleanSteps) { + expect(run, id).toContain(`awk '${filter}'`); + } }); it.skipIf(!awkAvailable)( From 1c87324c000f2f6cb92795a9effe51dd8c1cdeee Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 4 Aug 2026 23:52:56 +0800 Subject: [PATCH 07/10] fix(ci): harden review cleanup git calls --- .github/workflows/ci.yml | 39 ++++++++++--------- .github/workflows/qwen-code-pr-review.yml | 22 ++++++----- .../review-worktree-cleanup-workflow.test.js | 12 ++++++ 3 files changed, 45 insertions(+), 28 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cb1d581653d..392b4c6dd3b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -190,19 +190,20 @@ jobs: # script: this runs pre-checkout on shared runners, where leftover # workspace files are untrusted. if [ -e "$GITHUB_WORKSPACE/.git" ]; then - git -C "$GITHUB_WORKSPACE" worktree prune -v || true - git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ + 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 - git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || + "${GIT_SAFE[@]}" worktree remove --force "$worktree" || echo "::warning::could not remove review worktree: $worktree" done || true - git -C "$GITHUB_WORKSPACE" worktree prune -v || true - git -C "$GITHUB_WORKSPACE" 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 -C "$GITHUB_WORKSPACE" branch -D "$stale_ref" || + "${GIT_SAFE[@]}" branch -D "$stale_ref" || echo "::warning::could not remove review branch: $stale_ref" fi done || true @@ -568,19 +569,20 @@ jobs: # script: this runs pre-checkout on shared runners, where leftover # workspace files are untrusted. if [ -e "$GITHUB_WORKSPACE/.git" ]; then - git -C "$GITHUB_WORKSPACE" worktree prune -v || true - git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ + 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 - git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || + "${GIT_SAFE[@]}" worktree remove --force "$worktree" || echo "::warning::could not remove review worktree: $worktree" done || true - git -C "$GITHUB_WORKSPACE" worktree prune -v || true - git -C "$GITHUB_WORKSPACE" 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 -C "$GITHUB_WORKSPACE" branch -D "$stale_ref" || + "${GIT_SAFE[@]}" branch -D "$stale_ref" || echo "::warning::could not remove review branch: $stale_ref" fi done || true @@ -897,19 +899,20 @@ jobs: # script: this runs pre-checkout on shared runners, where leftover # workspace files are untrusted. if [ -e "$GITHUB_WORKSPACE/.git" ]; then - git -C "$GITHUB_WORKSPACE" worktree prune -v || true - git -C "$GITHUB_WORKSPACE" worktree list --porcelain \ + 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 - git -C "$GITHUB_WORKSPACE" worktree remove --force "$worktree" || + "${GIT_SAFE[@]}" worktree remove --force "$worktree" || echo "::warning::could not remove review worktree: $worktree" done || true - git -C "$GITHUB_WORKSPACE" worktree prune -v || true - git -C "$GITHUB_WORKSPACE" 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 -C "$GITHUB_WORKSPACE" branch -D "$stale_ref" || + "${GIT_SAFE[@]}" branch -D "$stale_ref" || echo "::warning::could not remove review branch: $stale_ref" fi done || true diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 777ecd06a18..24b0e84c690 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -397,15 +397,16 @@ 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" || true fi done || true - git worktree prune -v || true + "${GIT_SAFE[@]}" worktree prune -v || true echo "stale agent state cleaned" # SECURITY: checkout trusted base code; /review fetches PR diff context. @@ -1002,20 +1003,21 @@ jobs: exit 0 fi - git worktree prune -v || true - git worktree list --porcelain \ + 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 - git worktree remove --force "$worktree" || + "${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 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 review_ref; do [ -n "$review_ref" ] || continue - git branch -D "$review_ref" || + "${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 diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index adc454f813a..c67ae14927e 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -78,6 +78,15 @@ function expectCleanupRecipe(run) { expect(code.indexOf(`refs/heads/${branchFamily}*`)).toBeGreaterThan(remove); } +function expectHardenedGit(run) { + expect(run).toContain( + 'GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")', + ); + expect(run).not.toMatch( + /^\s+git(?:\s+-C\s+"\$GITHUB_WORKSPACE")?\s+(?:worktree|for-each-ref|branch)\b/m, + ); +} + const awkAvailable = spawnSync('awk', ['BEGIN { exit 0 }']).status === 0; describe('review worktree cleanup steps', () => { @@ -95,11 +104,13 @@ describe('review worktree cleanup steps', () => { `job "${id}" checks out on the shared pool and must clean stale .qwen state first`, ).toBeDefined(); expectCleanupRecipe(run); + expectHardenedGit(run); } }); it('keeps the review-job cleanup sweep pinned to paths.ts', () => { expectCleanupRecipe(reviewCleanStep); + expectHardenedGit(reviewCleanStep); // Fallback for worktree directories Git no longer knows about. expect(reviewCleanStep).toContain(`rm -rf ${worktreePrefix}*`); // Leases are session+prompt scoped so a stale one is inert, but the glob @@ -113,6 +124,7 @@ describe('review worktree cleanup steps', () => { // Directories are rm -rf'd first there, so no `worktree remove` to pin. expect(agentStateCleanStep).toContain(`rm -rf ${worktreePrefix}*`); expect(agentStateCleanStep).toContain(`refs/heads/${branchFamily}*`); + expectHardenedGit(agentStateCleanStep); }); it('uses one identical worktree filter at every list-driven sweep', () => { From 707aafb2fdae4c6584199ccd92d3dffed763f969 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 4 Aug 2026 16:39:32 +0000 Subject: [PATCH 08/10] fix(ci): tighten review cleanup comments and test guards (#8474) Co-authored-by: Qwen-Coder --- .github/workflows/ci.yml | 33 ++++++++++++------- .github/workflows/qwen-code-pr-review.yml | 2 ++ .../review-worktree-cleanup-workflow.test.js | 9 +++-- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 392b4c6dd3b..363ae9999bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -182,11 +182,14 @@ jobs: 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 for + # and qwen-review/* branches behind. prune drops registrations whose # directories the rm above removed; worktree remove --force then - # clears the directories that survived the rm (root-owned or - # read-only leftovers), since a branch checked out in a live - # worktree cannot be deleted. Kept inline rather than a shared + # 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 @@ -561,11 +564,14 @@ jobs: 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 for + # and qwen-review/* branches behind. prune drops registrations whose # directories the rm above removed; worktree remove --force then - # clears the directories that survived the rm (root-owned or - # read-only leftovers), since a branch checked out in a live - # worktree cannot be deleted. Kept inline rather than a shared + # 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 @@ -891,11 +897,14 @@ jobs: 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 for + # and qwen-review/* branches behind. prune drops registrations whose # directories the rm above removed; worktree remove --force then - # clears the directories that survived the rm (root-owned or - # read-only leftovers), since a branch checked out in a live - # worktree cannot be deleted. Kept inline rather than a shared + # 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 diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 24b0e84c690..b88b1411079 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -993,6 +993,8 @@ jobs: # 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 diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index c67ae14927e..e5831993abc 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -21,6 +21,9 @@ import { // renaming the layout there fails the build here instead of silently // no-op-ing the sweeps on the shared runners — a suffix rename already // broke a sweeper once (see paths.ts). +// npm-cache.yml and qwen-triage.yml also run on the shared pool but are +// deliberately not covered here; extending the sweep to them is follow-up +// work. const probePr = 12345; const toPosix = (value) => value.replace(/\\/g, '/'); const worktreePrefix = toPosix(worktreePath(probePr)).slice( @@ -39,8 +42,10 @@ const ciYaml = parse(readFileSync('.github/workflows/ci.yml', 'utf8')); const ciCleanSteps = Object.entries(ciYaml.jobs) .filter( ([, job]) => - JSON.stringify(job['runs-on']).includes('ubuntu_runner') && - job.steps.some((s) => String(s.uses ?? '').includes('actions/checkout')), + JSON.stringify(job['runs-on'] ?? '').includes('ubuntu_runner') && + (job.steps ?? []).some((s) => + String(s.uses ?? '').includes('actions/checkout'), + ), ) .map(([id, job]) => ({ id, From bce9a62bc157ec5da5d047971bf2ab236c7a121c Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 4 Aug 2026 18:48:08 +0000 Subject: [PATCH 09/10] test(ci): pin review cleanup recipe copies byte-identical (#8474) Co-authored-by: Qwen-Coder --- scripts/tests/review-worktree-cleanup-workflow.test.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index e5831993abc..03145d782a4 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -111,6 +111,15 @@ describe('review worktree cleanup steps', () => { expectCleanupRecipe(run); expectHardenedGit(run); } + // The copies are deliberate: a pre-checkout step cannot trust leftover + // workspace scripts, so the recipe stays inline per job. Pin them + // byte-identical so a fix to one sweep lands in all of them. + const [firstCopy, ...otherCopies] = ciCleanSteps; + for (const { id, run } of otherCopies) { + expect(run, `job "${id}" sweep drifted from the first copy`).toBe( + firstCopy.run, + ); + } }); it('keeps the review-job cleanup sweep pinned to paths.ts', () => { From f19e3faae75284f1a0d703c499c6b8a36ebe62bf Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 4 Aug 2026 23:07:00 +0000 Subject: [PATCH 10/10] fix(ci): guard review worktree removal and pin cleanup invariants (#8474) Co-authored-by: Qwen-Coder --- .github/workflows/ci.yml | 45 ++++++++++ .github/workflows/qwen-code-pr-review.yml | 18 +++- .../review-worktree-cleanup-workflow.test.js | 82 +++++++++++++++---- 3 files changed, 128 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 363ae9999bf..27c087d6e30 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -199,6 +199,21 @@ jobs: | 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 @@ -581,6 +596,21 @@ jobs: | 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 @@ -914,6 +944,21 @@ jobs: | 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 diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index b88b1411079..d1d74fbe2f5 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -403,7 +403,8 @@ jobs: "${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" || true + "${GIT_SAFE[@]}" branch -D "$stale_ref" || + echo "::warning::could not remove review branch: $stale_ref" fi done || true "${GIT_SAFE[@]}" worktree prune -v || true @@ -1011,6 +1012,21 @@ jobs: | 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 diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index 03145d782a4..281257875e5 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -37,31 +37,59 @@ const branchFamily = toPosix(reviewBranch(probePr)).slice( const ciYaml = parse(readFileSync('.github/workflows/ci.yml', 'utf8')); // Every ci.yml job that checks out on the shared self-hosted pool inherits a -// possibly dirty workspace. Enumerate by pool + checkout instead of job name -// so the next such job fails here instead of on the runners. +// possibly dirty workspace. Match the pool itself, not just the output +// reference that usually names it: jobs can also hard-code the shared label +// array, and a checkout on either form inherits the same leftovers. +// Enumerate by pool + checkout instead of job name so the next such job +// fails here instead of on the runners. const ciCleanSteps = Object.entries(ciYaml.jobs) .filter( ([, job]) => - JSON.stringify(job['runs-on'] ?? '').includes('ubuntu_runner') && + /ubuntu_runner|ecs-qwen/.test(JSON.stringify(job['runs-on'] ?? '')) && (job.steps ?? []).some((s) => String(s.uses ?? '').includes('actions/checkout'), ), ) .map(([id, job]) => ({ id, + steps: job.steps, run: job.steps.find((s) => s.name === 'Clean stale .qwen before checkout') ?.run, })); const reviewYaml = parse( readFileSync('.github/workflows/qwen-code-pr-review.yml', 'utf8'), ); -const reviewCleanStep = reviewYaml.jobs['review-pr'].steps.find( +const reviewCleanSteps = reviewYaml.jobs['review-pr'].steps; +const reviewCleanIndex = reviewCleanSteps.findIndex( (s) => s.name === 'Clean review worktrees', -).run; -const agentStateCleanStep = reviewYaml.jobs['review-pr'].steps.find( +); +const reviewCleanStep = reviewCleanSteps[reviewCleanIndex].run; +const agentStateCleanStep = reviewCleanSteps.find( (s) => s.name === 'Clean stale agent state', ).run; +// Comments may name the recipe pieces out of order when explaining them, so +// the order and isolation assertions below cover the commands only. +const stripComments = (run) => + run + .split('\n') + .filter((line) => !line.trim().startsWith('#')) + .join('\n'); + +// The steps run under `bash -e` + pipefail, and a failing for-each-ref or +// worktree-list head is exactly the corrupt-leftover state they exist to +// tolerate: every piped sweep loop must degrade to a warning via its +// trailing `|| true`, never fail the job. +function expectPipedLoopsIsolated(code, minLoops) { + const loops = + code.match(/\|\s*while read -r \w+; do[\s\S]*?\n\s*done(?: \|\| true)?/g) ?? + []; + expect(loops.length).toBeGreaterThanOrEqual(minLoops); + for (const loop of loops) { + expect(loop.endsWith('done || true')).toBe(true); + } +} + // prune (sync registrations) -> force-remove -> prune (drop now-stale // entries) -> delete branches: a branch checked out in a live worktree // cannot be deleted, so worktree removal must precede the branch sweep. @@ -69,27 +97,29 @@ function expectCleanupRecipe(run) { expect(run).toContain(`index($0, "/${worktreePrefix}")`); expect(run).toContain('worktree remove --force'); expect(run).toContain(`refs/heads/${branchFamily}*`); - // Order assertions below cover the commands only: comments may name the - // recipe pieces out of order when explaining them. - const code = run - .split('\n') - .filter((line) => !line.trim().startsWith('#')) - .join('\n'); + // The awk filter matches registered paths by substring, but those paths + // come from leftover git metadata and are untrusted: the removal loop + // must reject `..` traversal and re-anchor to the review prefix first. + expect(run).toContain('skipping suspicious review worktree path'); + expect(run).toContain(`"$GITHUB_WORKSPACE/${worktreePrefix}"*) : ;;`); + const code = stripComments(run); const remove = code.indexOf('worktree remove --force'); const firstPrune = code.indexOf('worktree prune'); expect(firstPrune).toBeGreaterThan(-1); expect(firstPrune).toBeLessThan(remove); expect(code.indexOf('worktree prune', remove)).toBeGreaterThan(remove); expect(code.indexOf(`refs/heads/${branchFamily}*`)).toBeGreaterThan(remove); + expectPipedLoopsIsolated(code, 2); } function expectHardenedGit(run) { expect(run).toContain( 'GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")', ); - expect(run).not.toMatch( - /^\s+git(?:\s+-C\s+"\$GITHUB_WORKSPACE")?\s+(?:worktree|for-each-ref|branch)\b/m, - ); + // Any column, any verb: the review-workflow copies are unindented after + // YAML block-scalar stripping, and a bare `git` call would run un-hardened + // against leftover untrusted .git config. + expect(run).not.toMatch(/^\s*git\s/m); } const awkAvailable = spawnSync('awk', ['BEGIN { exit 0 }']).status === 0; @@ -103,11 +133,25 @@ describe('review worktree cleanup steps', () => { 'integration_cli', ]), ); - for (const { id, run } of ciCleanSteps) { + for (const { id, steps, run } of ciCleanSteps) { expect( run, `job "${id}" checks out on the shared pool and must clean stale .qwen state first`, ).toBeDefined(); + // Position is load-bearing: the sweep must run after the ownership + // restore (git refuses root-owned leftovers) and before checkout + // (after checkout it no-ops on the fresh tree). + 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, id).toBeGreaterThan(restoreIdx); + expect(cleanIdx, id).toBeLessThan(checkoutIdx); expectCleanupRecipe(run); expectHardenedGit(run); } @@ -123,6 +167,11 @@ describe('review worktree cleanup steps', () => { }); it('keeps the review-job cleanup sweep pinned to paths.ts', () => { + // `always()` and the end-of-job position are what make the step fire on + // the failure/cancellation paths it exists for: Actions' default + // success() condition would skip it once any earlier step fails. + expect(reviewCleanSteps[reviewCleanIndex].if).toBe('always()'); + expect(reviewCleanIndex).toBe(reviewCleanSteps.length - 1); expectCleanupRecipe(reviewCleanStep); expectHardenedGit(reviewCleanStep); // Fallback for worktree directories Git no longer knows about. @@ -139,6 +188,7 @@ describe('review worktree cleanup steps', () => { expect(agentStateCleanStep).toContain(`rm -rf ${worktreePrefix}*`); expect(agentStateCleanStep).toContain(`refs/heads/${branchFamily}*`); expectHardenedGit(agentStateCleanStep); + expectPipedLoopsIsolated(stripComments(agentStateCleanStep), 1); }); it('uses one identical worktree filter at every list-driven sweep', () => {