fix(ci): skip non-bot review_requested siblings before jobs spend compute - #9204
Merged
Conversation
…pute 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 (five within the same second on #8830/#9142). Only the bot-requested run can reach review-pr; the human-requested siblings used to spend a review-config runner plus an authorize job (CI_BOT_PAT permission API) each before no-op exiting. Mirror the requested_reviewer predicate precheck-pr already applies to fork PRs into authorize.if and review-config.if so the siblings complete as instant all-skipped runs.
github-actions
Bot
added
the
review/self-reported
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template: looks good ✓ (minor nit: the body omits the template's Chinese Problem: observed, not theoretical. #8945 documents the burst with run-history evidence (five Direction: aligned. The issue thread weighed three levers, and a maintainer explicitly called option (2) — move the Size: no core module paths touched — 18 production lines in Approach: scope feels right. The two Risk: no elevated-risk path matches. One thing to state plainly: this edits the bot's own review pipeline, so a mis-predicate would only surface on live events post-merge — but the PR's reachable-path matrix checks out against the workflow's Moving on to code review. 🔍 中文说明感谢贡献！ 模板：完整 ✓（小问题：正文缺少模板中的中文 问题：已观测到，非理论问题。#8945 附有 run 历史证据（#8830 open 时同一秒内 5 个 方向：对齐。issue 讨论权衡了三种方案，maintainer 明确方案 (2)——把 规模：未触及核心模块路径—— 方案：范围合理。两处 风险：未命中高风险路径。需要直说：这改的是 bot 自己的评审流水线，谓词若有误只会在合并后的真实事件上暴露——但 PR 描述的可达路径矩阵与 workflow 的 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent proposal first: given the title and motivation I would (a) mirror What I verified against the base workflow:
No critical findings, no convention violations. Not verified: the actual GitHub-side routing (siblings completing as all-skipped runs) is only observable on the next same-repo bot PR open post-merge — that's inherent to workflow changes, and the PR's Risk & Scope already says so. Testing evidence (unattended CI run — PR's own CI via API; no PR code executed here)The Linux unit suite — the only job that runs Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remaining pin-test claim: 中文说明代码审查：先独立给出方案再对比——我会把 precheck-pr 的 对照 base 版 workflow 验证了两点：其一，跳过的 测试证据（无人值守 CI，仅通过 API 读取 PR 自身 CI，未执行任何 PR 代码）：Linux 单元测试（唯一运行新 pin 测试的 job）抓取时仍在进行；macOS/Windows 单测与集成测试因仓库 CI 分类（本次仅改 scripts 与 workflow YAML）被跳过，属常规形态而非失败；抓取时无红色检查。作者"本地 54 个文件通过"是作者自述，不作为证据。 沙箱验证可补齐最后一项： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; this is exactly what a well-scoped CI fix looks like. Stepping back: the problem is observed and quantified (five same-second The one way this PR could have broken things — a skipped Reservations: none blocking. The actual all-skipped sibling runs can only be observed post-merge (on the next same-repo bot PR open), which is inherent to workflow changes and honestly scoped in the PR body; Approval deferred until CI lands green on 中文说明总体评价：各阶段均干净。问题已观测且量化（bot PR open 时同一秒 5 个 唯一可能出问题的地方——被跳过的 无阻塞性保留意见。兄弟 run 全跳过的实际效果只能在合并后观测（下一次同仓库 bot PR open 时），这是 workflow 改动的固有属性，PR 正文已如实说明；如需先 A/B 证明 pin 测试"先红后绿"，maintainer 可触发 Linux 单元测试（运行新 pin 测试的 job）在审查时仍在进行，待 CI 在该提交上全绿后由 finalize 流程完成批准。 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Aug 15, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: verification and reverse audit — each ran and opened its brief, but neither was launched with the prompt the CLI built — the launches were written by hand, so the posted findings cannot be counted as verified, and what the agents were actually asked is not what this skill certifies.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
— qwen3.8-max via Qwen Code /review (v0.21.12)
Comment on lines
+167
to
+168
| # SYNC with the review_requested clauses in precheck-pr.if and | ||
| # authorize.if, and with the bot_login constant below. |
Collaborator
There was a problem hiding this comment.
[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)
Comment on lines
+2557
to
+2558
| const cond = doc.jobs['review-config'].if; | ||
| expect(cond).toContain("github.event.action == 'review_requested'"); |
Collaborator
There was a problem hiding this comment.
[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)
wenshao
reviewed
Aug 15, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. 3 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
— glm-5.3 via Qwen Code /review (v0.21.12)
wenshao
approved these changes
Aug 15, 2026
qwen-code-dev-bot
added a commit
that referenced
this pull request
Aug 15, 2026
Resolve the EOF conflict in qwen-pr-review-workflow.test.js by keeping both appended suites: this PR's concurrency-routing tests and main's #9204 review_requested burst-coalescing tests. The workflow changes are orthogonal: the concurrency group routes non-bot review_requested runs to per-run groups (this PR) while review-config.if and authorize.if skip those siblings before they spend compute (#9204). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This was referenced Aug 15, 2026
pull Bot
pushed a commit
to bhardwajRahul/qwen-code
that referenced
this pull request
Aug 16, 2026
…te and body sentence (QwenLM#9215) * fix(review): give duplicate-dropped Suggestions their own compose state and body sentence A review whose confirmed Suggestions were dropped because the PR already carried them (prior round or concurrent reviewer) had nowhere to record the drop except suggestionsDiscarded — whose body sentence asserts an anchor failure. On QwenLM#9204 resolve-anchors returned three exact-added matches, the drop reason was duplication, and the posted body claimed the findings "could not be anchored to a changed line" — a public claim the run's own artifacts contradict. Add a suggestionsDroppedAsDuplicates state field: entries name the finding and where it already lives, render as their own body paragraph (bilingual, comment refs linkified), and count toward S exactly like anchor-failure discards so an all-duplicate run never reads as zero-finding. The skill's Step 7 state list now routes duplicate drops to it instead of the count. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): hoist plan PR-identity read; pin duplicate-clause zh count and empty-entry filter * fix(review): duplicate-drop account renders on every event; one stripped-list helper * fix(review): cap-free duplicate-drop fixtures, honest seam pin, body-rule carve-out * test(review): pin the full duplicate list and the compose-review --input seam * fix(review): bound the duplicate-drop account like the deferred channel * fix(review): drop truncated comment refs, collapse CRs, bound cannot-tell (QwenLM#9215) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Collaborator
|
Released in v0.21.13. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What this PR does
Mirrors the
requested_reviewerpredicate thatprecheck-pralready applies to fork PRs into the job-levelifs ofauthorizeandreview-configinqwen-code-pr-review.yml: areview_requestedevent now only spends those jobs when the bot itself is the requested reviewer. Human-requested sibling runs complete as instant all-skipped runs — no permission-API call, no runner slot — exactly option (2) from the issue discussion. The bot login literal stays sourced fromreview-config'sbot_loginconstant in the new pin test.Why it's needed
Opening a same-repo PR that touches CODEOWNERS-covered paths auto-requests every owner individually, so one PR open emits one
review_requestedrun per owner — five within the same second on #8830, reproduced again on #9142's open (five human review_requested events at 08:12:39-40Z).precheck-pr's identical predicate only fires for fork PRs, so on same-repo PRs each sibling run currently spends areview-configrunner plus anauthorizejob (which loads CI_BOT_PAT and calls the collaborator-permission API) beforereview-prno-op exits on its ownrequested_reviewer == bot_logincheck.Reviewer Test Plan
How to verify
Behavior matrix (unchanged outcomes, less compute):
review_requested, bot requested →authorizeruns its permission check andreview-prproceeds as today.review_requested, human/team requested →authorizeandreview-configskip;review-prskips becauseneeds.authorize.outputs.should_reviewis empty (it skipped before too, just later).review_requested→precheck-prkeeps its existing predicate;authorize's new clause passes for bot requests and adds a second skip reason for human requests (already skipped via the missingallow_triagedecision).opened/synchronize/reopened/ready_for_review, comment and review commands,workflow_dispatch→ the new clause is vacuously true (action != 'review_requested'orevent_name != 'pull_request_target'); no routing change.New pins in
scripts/tests/qwen-pr-review-workflow.test.js(red before, green after): theauthorizedisjunction is present verbatim,review-configis gated on the bot login, and the literal is extracted from thebot_loginstep rather than paraphrased.npm run test:scripts # 54 files pass locally (see Risk & Scope for one env note)Evidence (Before & After)
Before: #8830's open produced six runs with five cancellations (issue body); #9142's timeline shows five same-second human review_requested events whose sibling runs each spent
authorize. After: the same burst evaluates to all-skipped sibling runs; observable on the next same-repo bot PR open post-merge.Tested on
Environment (optional)
Workflow YAML pins via vitest (
scripts/tests). actionlint runs in CI; the binary was not reachable from this machine while preparing the PR.Risk & Scope
1df9c5dera) — CODEOWNERS requests don't re-fire on push, so that one needs re-observation on a future bot push, per the issue thread. Dismissing CODEOWNERS auto-requests at creation time (the CODEOWNERS-side lever) is deliberately not touched.install-script.test.jsfails in a fresh worktree untilpackages/audio-captureis built (pre-existing on main, verified via stash baseline); passes here afternpm run build -w packages/audio-capture.Linked Issues
Fixes #8945