Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
18 changes: 17 additions & 1 deletion .github/workflows/qwen-code-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,17 @@ jobs:
fi

review-config:
# Bot-requested review_requested only: a CODEOWNERS-covered PR open
# auto-requests every owner individually (#8945), spawning one
# review_requested run per owner. Only the run where the bot itself is
# the requested reviewer can reach review-pr, so the human-requested
# siblings must skip here instead of each spending a runner. KEEP IN
# SYNC with the review_requested clauses in precheck-pr.if and
# authorize.if, and with the bot_login constant below.
Comment on lines +167 to +168

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] This new KEEP IN SYNC comment names three sync sites (precheck-pr.if, authorize.if, and the bot_login constant), but the pin tests this PR adds enforce only two — nothing pins precheck-pr.if's pre-existing requested_reviewer.login == 'qwen-code-ci-bot' predicate. — Failure scenario: a future bot-login rename (or a one-sided clause edit) updates the three pinned sites and passes CI, leaving precheck-pr.if stale; on a fork PR where the bot is requested for review, precheck-pr then skips, decision is never 'allow_triage', authorize's fork gate fails, and the bot silently stops reviewing fork PRs with no test failing.

Pin the third site too so all three fail together on drift:

expect(doc.jobs['precheck-pr'].if).toContain(
  `github.event.requested_reviewer.login == '${botLogin}'`,
);

— qwen3.8-max via Qwen Code /review (v0.21.12)

if: |-
github.event_name == 'pull_request_target' &&
github.event.action == 'review_requested'
github.event.action == 'review_requested' &&
github.event.requested_reviewer.login == 'qwen-code-ci-bot'
runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}'
permissions: {}
outputs:
Expand Down Expand Up @@ -225,13 +233,21 @@ jobs:
# Only run for PR-target events and supported command comments — not every
# unrelated comment — to avoid spawning a job per comment. The downstream
# `if`s still do the exact command body match; this prefix is just a filter.
# review_requested must additionally request the bot itself: a
# CODEOWNERS-covered PR open auto-requests every owner individually
# (#8945), and precheck-pr's identical predicate only covers fork PRs —
# without this clause each same-repo sibling run spends an authorize job
# (permission API + runner slot) before review-pr no-op exits.
if: |-
!cancelled() &&
(github.event_name != 'pull_request_target' ||
github.event.action != 'closed') &&
(github.event_name != 'pull_request_target' ||
github.event.pull_request.head.repo.full_name == github.repository ||
needs.precheck-pr.outputs.decision == 'allow_triage') &&
(github.event_name != 'pull_request_target' ||
github.event.action != 'review_requested' ||
github.event.requested_reviewer.login == 'qwen-code-ci-bot') &&
(github.event_name == 'pull_request_target' ||
(github.event_name == 'workflow_dispatch' &&
github.event.inputs.command == 'resolve') ||
Expand Down
44 changes: 44 additions & 0 deletions scripts/tests/qwen-pr-review-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2517,3 +2517,47 @@ describe('bot comment markers', () => {
expect(ackLine).toContain('"$RUN_URL"');
});
});

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
// per owner (observed: five within the same second on #8830/#9142). Only
// the bot-requested run can reach `review-pr`; the human-requested siblings
// used to spend an `authorize` job (permission API + runner slot) each
// before no-op exiting. `authorize` and `review-config` must filter those
// 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' \|\|`,
String.raw`\s*github\.event\.action != 'review_requested' \|\|`,
String.raw`\s*github\.event\.requested_reviewer\.login == '${botLogin}'\s*\)`,
].join(''),
);

it('resolves the bot login from review-config, not a paraphrase', () => {
expect(botLogin).toBe('qwen-code-ci-bot');
});

it('filters non-bot review_requested events before authorize spends compute', () => {
// The same disjunction precheck-pr already applies to fork PRs; mirroring
// it here covers same-repo PRs, which precheck-pr deliberately skips.
expect(doc.jobs['authorize'].if).toMatch(botRequestedClause);
});

it('keeps review-config from running for non-bot review_requested events', () => {
// review-config exists to feed bot_login into review-pr; only the
// bot-requested run can get there, so every other sibling is pure waste.
const cond = doc.jobs['review-config'].if;
expect(cond).toContain("github.event.action == 'review_requested'");
Comment on lines +2557 to +2558

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] These pin tests assert fragments of review-config.if (the atomic comparisons via toContain) rather than the full normalized condition, so small mutations survive the suite green while reviving the exact compute waste this PR removes — or worse. — Failure scenario (mutation-probed against the real suite): flipping the && between the action and bot-login comparisons to || lets human-requested siblings pass again with both toContains green; and inverting the leading github.event_name == 'pull_request_target' to != makes review-config skip every pull_request_target run so bot_login resolves to '' and the bot silently stops every review-requested review with all 116 tests green. The authorize.if test below has the same gap — flipping the && after the new clause to || survives toMatch.

Pin the full normalized expression rather than fragments, e.g.:

expect(cond.replace(/\s+/g, ' ')).toBe(
  "github.event_name == 'pull_request_target' && " +
    "github.event.action == 'review_requested' && " +
    `github.event.requested_reviewer.login == '${botLogin}'`,
);

and extend botRequestedClause to require the trailing &&. witness: each mutation keeps the shipped tests green (Tests 3 passed | 113 skipped / Tests 116 passed); the full-expression pin catches the mutation (Tests 1 failed | 115 passed) and passes on the correct workflow (Tests 116 passed).

— qwen3.8-max via Qwen Code /review (v0.21.12)

expect(cond).toContain(
`github.event.requested_reviewer.login == '${botLogin}'`,
);
});
});
Loading