diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index cac7261e618..1090eec7dff 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -50,10 +50,23 @@ on: concurrency: # PR lifecycle events share a PR-scoped group so new pushes restart the delay - # and closed PRs stop any in-flight lifecycle review. - # Comment/review events use per-run groups to avoid cancelling active reviews. + # and closed PRs stop any in-flight lifecycle review. Every review_requested + # run — the bot-directed one included — gets a per-run group: membership is + # decided here, before `authorize` runs, but whether a bot request reviews + # anything is `authorize`'s call on the REQUESTER's write permission. A + # requester without write produces a guaranteed all-skipped run, and as a + # shared-group member that no-op can supersede a lifecycle run sitting + # PENDING behind a still-terminating review — a pending run is replaced by + # any newer run in the group, cancel-in-progress notwithstanding. That is + # the exact race that lost the automatic review on PR #9091, left open for + # anyone who can request the bot without write permission. The per-run group + # costs only an occasional duplicate review when an authorized bot request + # lands while the lifecycle run for the same head still queues: compute, + # never a lost review. Comment/review events use per-run groups to avoid + # cancelling active reviews. group: >- ${{ github.event_name == 'pull_request_target' && + github.event.action != 'review_requested' && format('qwen-pr-review-pr-{0}', github.event.pull_request.number) || format('qwen-pr-review-run-{0}', github.run_id) }} cancel-in-progress: "${{ github.event_name == 'pull_request_target' && (github.event.action == 'synchronize' || github.event.action == 'closed') }}" diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 5af2cfb0c69..b9af51489a1 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -31,6 +31,13 @@ const workflowFiles = readdirSync(workflowsDir).filter((f) => /\.ya?ml$/.test(f), ); +// Single shared recipe for the review-config bot login; every suite that pins +// it reads this constant so the extraction cannot drift between call sites. +const botLogin = + parse(workflow) + .jobs['review-config'].steps.find((s) => s.name === 'Set review constants') + ?.run.match(/bot_login=([A-Za-z0-9-]+)/)?.[1] ?? ''; + function runReviewStep() { const doc = parse(workflow); const step = doc.jobs['review-pr'].steps.find((s) => s.name === 'Run review'); @@ -2518,6 +2525,50 @@ describe('bot comment markers', () => { }); }); +describe('qwen pr review concurrency routing', () => { + // A PENDING run in a concurrency group is replaced by any newer run of the + // same group — cancel-in-progress does not protect it. On PR #9091 a push + // and three human review requests landed in the same minute: the + // synchronize run cancelled the in-flight review but queued behind it while + // it terminated, and the review_requested runs — no-ops, since review-pr + // only runs when the bot itself is the requested reviewer — superseded it + // while pending. The sole survivor skipped review-pr, so the push was never + // reviewed. Routing only the human-requested siblings to a per-run group + // leaves the race open: group membership is fixed here, before `authorize` + // runs, but whether a bot-directed request reviews anything is `authorize`'s + // call on the REQUESTER's write permission — a requester without write + // produces a guaranteed all-skipped run, and as a shared-group member that + // no-op can supersede the pending lifecycle run, losing the review the same + // way. Every review_requested run therefore gets a per-run group; an + // authorized bot request still reviews immediately, it just can no longer + // supersede the lifecycle run for the same head. + const group = parse(workflow).concurrency.group; + + it('keeps every review_requested run out of the shared PR group', () => { + // Verbatim, like the cancel-in-progress pin in the resolve suite: the + // shape IS the fix — `&&` binds tighter than `||`, so exactly the + // lifecycle actions reach the PR group and every review_requested (bot + // included) falls through to the per-run group. Any requested_reviewer + // clause here re-admits the unauthorized-requester no-op and silently + // re-ships the race. + expect(group).toBe( + "${{ github.event_name == 'pull_request_target' && " + + "github.event.action != 'review_requested' && " + + "format('qwen-pr-review-pr-{0}', github.event.pull_request.number) || " + + "format('qwen-pr-review-run-{0}', github.run_id) }}", + ); + }); + + it('gates the review_requested jobs on the published bot login', () => { + // The group expression no longer names the requested reviewer; the + // job-level gates still must. Pin the surviving literal against the + // review-config constant so a bot rename cannot desync the sites. + expect(parse(workflow).jobs['precheck-pr'].if).toContain( + `github.event.requested_reviewer.login == '${botLogin}'`, + ); + }); +}); + describe('review_requested burst coalescing (#8945)', () => { // Opening a same-repo PR that touches CODEOWNERS-covered paths auto-requests // every owner individually, so one PR open emits one `review_requested` run @@ -2528,11 +2579,6 @@ describe('review_requested burst coalescing (#8945)', () => { // siblings at the job level so they complete as instant all-skipped runs. const doc = parse(workflow); - const botLoginMatch = doc.jobs['review-config'].steps[0].run.match( - /bot_login=([A-Za-z0-9-]+)/, - ); - const botLogin = botLoginMatch ? botLoginMatch[1] : ''; - const botRequestedClause = new RegExp( [ String.raw`\(\s*github\.event_name != 'pull_request_target' \|\|`,