Repository navigation
fix(ci): clean review worktrees after cancellation #8474
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
Merged
Merged
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
41d4c66
fix(ci): clean review worktrees after cancellation
yiliang114 d1f976a
fix(ci): remove orphaned review worktree directories
yiliang114 772d71c
Merge remote-tracking branch 'origin/main' into codex/cleanup-review-…
yiliang114 02909be
fix(tests): sync qwen-resolve-workflow expectations with externalized…
yiliang114 0a8d999
fix(ci): pin review worktree cleanup patterns to paths.ts (#8474)
qwen-code-dev-bot 168bbe6
Merge branch 'main' into codex/cleanup-review-worktrees
qwen-code-dev-bot dbdcd7d
Merge remote-tracking branch 'origin/main' into codex/cleanup-review-…
qwen-code-dev-bot 3e70e8d
Merge branch 'main' into codex/cleanup-review-worktrees
yiliang114 0065bb8
Merge branch 'main' into codex/cleanup-review-worktrees
yiliang114 eb1f6a6
fix(ci): harden review cleanup sweeps and cover integration_cli (#8474)
qwen-code-dev-bot f7f0954
fix(ci): extend review cleanup sweep to web_shell_e2e_smoke (#8474)
qwen-code-dev-bot 1c87324
fix(ci): harden review cleanup git calls
yiliang114 707aafb
fix(ci): tighten review cleanup comments and test guards (#8474)
yiliang114 bce9a62
test(ci): pin review cleanup recipe copies byte-identical (#8474)
qwen-code-dev-bot f19e3fa
fix(ci): guard review worktree removal and pin cleanup invariants (#8…
qwen-code-dev-bot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| /** | ||
| * @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 { | ||
| 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 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( | ||
| 0, | ||
| -`${probePr}`.length, | ||
| ); | ||
| const branchFamily = toPosix(reviewBranch(probePr)).slice( | ||
| 0, | ||
| -`pr-${probePr}`.length, | ||
| ); | ||
|
|
||
| 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. | ||
| 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'), | ||
| ); | ||
| 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 | ||
| // 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}*`); | ||
| // 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(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 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', () => { | ||
| // 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(); | ||
| for (const { id, run } of ciCleanSteps) { | ||
| expect(run, id).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); | ||
| }, | ||
| ); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 addsites 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 itsrm -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 manualgit worktree removeclears 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 恢复逻辑，把它与守卫组合起来后我们很乐意再议。