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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion .github/workflows/qwen-code-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,21 @@ 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.
# and closed PRs stop any in-flight lifecycle review. A bot-directed
# review_requested joins that group too: it is an explicit review ask and
# must coalesce with the lifecycle run for the same head. Any OTHER
# review_requested (a human or team reviewer) gets a per-run group: it no-ops
# at review-pr by design, but as a member of the shared group it can
# supersede a lifecycle run that sits PENDING behind a still-terminating
# review — a pending run is replaced by any newer run in the group,
# cancel-in-progress notwithstanding. That race lost the automatic review
# entirely on PR #9091: a push plus three human review requests in the same
# minute cancelled the in-flight review and left only the no-op request run.
# Comment/review events use per-run groups to avoid cancelling active reviews.
group: >-
${{ github.event_name == 'pull_request_target' &&
(github.event.action != 'review_requested' ||
github.event.requested_reviewer.login == 'qwen-code-ci-bot') &&
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') }}"
Expand Down
55 changes: 50 additions & 5 deletions scripts/tests/qwen-pr-review-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -2518,6 +2525,49 @@ 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 — guaranteed 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. The fix routes non-bot review_requested runs to
// a per-run group where they can supersede nothing, while lifecycle events
// and bot-directed requests keep sharing the PR group so an explicit bot
// ask still coalesces with the automatic run for the same head.
const group = parse(workflow).concurrency.group;

it('keeps no-op review requests 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 and the bot-directed request reach the PR group, and
// every other review_requested (human or team reviewer) falls through to
// the per-run group. Moving the gate behind the fallback, or dropping the
// parentheses around the disjunction, silently re-ships the race.
expect(group).toBe(
"${{ github.event_name == 'pull_request_target' && " +
"(github.event.action != 'review_requested' || " +
"github.event.requested_reviewer.login == 'qwen-code-ci-bot') && " +
"format('qwen-pr-review-pr-{0}', github.event.pull_request.number) || " +
"format('qwen-pr-review-run-{0}', github.run_id) }}",
);
});

it('gates on the same bot login the workflow publishes', () => {
// The group expression cannot read review-config's bot_login output, so
// it carries the literal; pin it against the constant it mirrors so a
// rename of the bot cannot desync all three sites.
expect(group).toContain(
`github.event.requested_reviewer.login == '${botLogin}'`,
);
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
Expand All @@ -2528,11 +2578,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' \|\|`,
Expand Down
Loading