fix(ci): gate PR review and triage on write permission - #5191
Merged
Conversation
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: the On approach: scope is tight — two workflow files, each getting one Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向： 方案：范围紧凑——两个 workflow 文件，各新增一个 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the bundled /review skill prompt to support a CI “emit-only” mode, where the agent writes review payload JSON to a file instead of calling the GitHub Reviews API directly, enabling least-privilege CI publishing in a separate step.
Changes:
- Add a Step 4 emit-only fallback that prewrites a provisional “review-unavailable” payload to
QWEN_REVIEW_OUTPUT_FILEbefore launching parallel agents. - Add Step 9 branching to either submit via
gh api(default) or atomically write the review JSON toQWEN_REVIEW_OUTPUT_FILE(emit-only), for both findings and no-findings flows.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Code ReviewReviewed the diff against an independent proposal. The PR's approach matches what I'd have written — a single No blockers found. Specific findings:
One minor observation (non-blocking): the Testingtmux real-scenario testing: N/A — this PR modifies GitHub Actions workflow files (CI infrastructure), not user-visible CLI behavior. There is no CLI command to drive. Verification is via YAML parse, bash -n, and end-to-end logic tracing across all trigger paths ( 中文说明代码审查对照独立方案审查 diff。PR 的方法与我预期一致——每个 workflow 一个 未发现阻塞项。 具体发现：
一个小观察（非阻塞）： 测试tmux 真实场景测试：N/A——本 PR 修改的是 GitHub Actions workflow 文件（CI 基础设施），不是用户可见的 CLI 行为。没有可驱动的 CLI 命令。验证方式为 YAML 解析、bash -n、以及对所有触发路径（ — Qwen Code · qwen3.7-max |
Collaborator
|
This is a solid infrastructure PR. Clean design, minimal scope, correct implementation. The motivation is genuine — The implementation holds up under scrutiny. Principal resolution covers every trigger path correctly. Fail-closed behavior means errors and missing permissions deny rather than silently allow. The The only minor note is that Approving. ✅ 中文说明这是一个扎实的基础设施 PR。设计干净，范围最小，实现正确。 动机是真实的—— 实现在审查下站得住脚。主体解析正确覆盖每条触发路径。失败关闭行为确保错误和缺失权限会拒绝而非静默放行。 唯一的小注意点是 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jun 16, 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. ✅
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. |
Replace the per-path author_association checks (and the same-repo-only fork check in triage) with a dedicated `authorize` job that resolves the triggering principal — the PR author for automatic PR events, the commenter for /review and /triage commands, the requester for review_requested — and verifies it has admin/maintain/write permission via the collaborators API. This lets fork PRs by write-permission authors qualify for automatic review and triage (author_association reports such authors as CONTRIBUTOR from a fork, so they were previously skipped), and tightens command triggers to real write permission instead of the author_association proxy. The gate fails closed: any API error or non-write permission denies the run. The authorize job uses CI_BOT_PAT because reading collaborator permission requires write/maintain/admin access; it runs no agent, checks out nothing, and processes no untrusted PR content, so holding the PAT there is safe. External actors without write permission still cannot trigger the model-calling jobs.
yiliang114
force-pushed
the
feat/review-emit-only
branch
from
June 16, 2026 11:18
68c3ebf to
c7db5f0
Compare
yiliang114
changed the title
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jun 16, 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. ✅
Collaborator
Author
Validation of the write-permission gateThe 1. Gate decision — real users, the exact API the job uses. Ran
On a public repo, non-collaborators resolve to 2. Current behavior, from real runs.
So the real behavioral change is triage now covering write users' fork PRs; review moves onto the same write-permission check for consistency. 3. Branch workflow dry-run (run 4. Not validated until merge (mechanism limit). The gate's live allow/deny only runs on real events, which use the base-branch workflow. After merge, confirm on the first real triggers: a write user's fork PR → Migration note: the job |
qwen-code-ci-bot
previously requested changes
Jun 16, 2026
Collaborator
|
tmux real-user testing: pass - workflow run Launched the changed app in a real tmux session and exercised the affected flow. E2E test report— Qwen Code · tmux real-user testing |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
The core authorization logic is sound — fail-closed design, correct principal selection per event type, and the collaborator-permission API check is strictly more reliable than author_association for fork PRs. YAML parses clean, bash syntax validates, build and typecheck pass.
Several low-confidence findings need human judgment (not posted as inline comments):
- Silent PAT failure: When
CI_BOT_PATexpires or is rate-limited, the authorize job exits 0 withshould_review=false. The workflow run shows green in the Actions list — no red X. The::error::annotation is buried in step details. Considerexit 1on API failure to make infrastructure outages visible. - No user feedback on PAT failure:
ack-review-requestskips when PAT fails (noalways()). Users who type@qwen-code /reviewreceive zero feedback — the bot silently ignores them. This is a regression from the old code where ack could post independently. - Duplicated authorize scripts: ~30 lines of near-identical bash in both workflows will diverge over time. A composite action would eliminate this.
- Authorize fires on non-PR issue comments: The
issue_commentbranch of the authorizeifis missinggithub.event.issue.pull_request, so a/reviewcomment on a regular issue spawns the authorize job unnecessarily. - Org member permission narrowing: Old
MEMBERcheck authorized any org member; new collaborators API requires explicitwrite+. Org members with onlyread/triageare silently denied.
— qwen3.7-max via Qwen Code /review
yiliang114
dismissed
qwen-code-ci-bot’s stale review
June 16, 2026 16:51
All review comments have been addressed and the threads resolved. The two findings on current code were verified as non-issues (the github.repository guard is present at line 179; the reopened/ready_for_review path runs immediately via short-circuit, matching the comment). Dismissing this stale review — later automated reviews are COMMENTED only and don't supersede it.
Collaborator
Author
|
@qwen-code /review |
Contributor
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/27633888607)._ |
| echo "::error::Permission API call failed for ${principal}: ${api_error}" | ||
| echo "Failed to check permission for ${principal} (API error: ${api_error}); denying." >> "$GITHUB_STEP_SUMMARY" | ||
| echo "should_review=false" >> "$GITHUB_OUTPUT" | ||
| exit 0 |
Collaborator
There was a problem hiding this comment.
[Suggestion] The exit 0 here makes API failures (PAT expiry, rate limits, GitHub outages) indistinguishable from legitimate permission denials at the job level. The authorize job shows a green checkmark while all downstream jobs silently skip. An oncall engineer must drill into the step summary to determine the root cause.
Consider differentiating transient from permanent failures:
Suggested change
| exit 0 | |
| # Distinguish transient API errors from permanent denials. | |
| # Retry on transient errors (429, 5xx); fail the job so the red X | |
| # propagates to downstream jobs and alerts oncall. | |
| if grep -qE 'HTTP (429|500|502|503|504)' <<< "$api_error"; then | |
| echo "::error::Transient API error for ${principal}: ${api_error}" | |
| echo "Failed to check permission for ${principal} (transient API error); failing job." >> "$GITHUB_STEP_SUMMARY" | |
| exit 1 | |
| fi | |
| echo "::error::Permission API call failed for ${principal}: ${api_error}" | |
| echo "Failed to check permission for ${principal} (API error: ${api_error}); denying." >> "$GITHUB_STEP_SUMMARY" | |
| echo "should_review=false" >> "$GITHUB_OUTPUT" | |
| exit 0 |
Additionally, a preliminary token validity check (e.g., gh api /user) at the start of the step would make PAT expiry immediately obvious with a clear "rotate the secret" message.
— qwen3.7-max via Qwen Code /review
| echo "::error::Permission API call failed for ${principal}: ${api_error}" | ||
| echo "Failed to check permission for ${principal} (API error: ${api_error}); denying." >> "$GITHUB_STEP_SUMMARY" | ||
| echo "should_run=false" >> "$GITHUB_OUTPUT" | ||
| exit 0 |
Collaborator
There was a problem hiding this comment.
[Suggestion] Same pattern as the review workflow — exit 0 on API failure makes the authorize job succeed silently when the GitHub API is down or CI_BOT_PAT has expired. Oncall sees green checkmarks but no reviews or triage runs happen.
Consider:
- Distinguishing transient errors (429, 5xx) and letting the job fail (
exit 1) so the red X propagates. - Adding a preliminary
gh api /usercheck at the start to detect PAT expiry immediately with a clear "rotate CI_BOT_PAT" error message. - Extracting the shared permission-check script (~30 lines, nearly identical in both files) into a reusable composite action to avoid drift and simplify future fixes like this one.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
approved these changes
Jun 16, 2026
|
|
||
| authorize-review-request: | ||
| needs: ['review-config'] | ||
| authorize: |
Collaborator
There was a problem hiding this comment.
[Suggestion] The authorize shell script (~35 lines — principal resolution, mktemp/gh api error capture, \r/\n sanitization, and admin|maintain|write case) is duplicated nearly verbatim in qwen-triage.yml. A bug fix or improvement (e.g., adding retry for rate limits, a new permission level) must be patched in both copies independently.
Consider extracting into a reusable composite action:
# .github/actions/check-permission/action.yml
inputs:
principal:
required: true
output-name:
default: should_run
runs:
using: composite
steps:
- shell: bash
env:
GH_TOKEN: ${{ inputs.token }}
PRINCIPAL: ${{ inputs.principal }}
run: |
# shared authorize logic hereEach workflow keeps only its principal-selection case (which legitimately differs) and calls the shared action.
— qwen3.7-max via Qwen Code /review
| # KEEP IN SYNC with review-pr.if (explicit-trigger branches). | ||
| # Authorization is delegated to the `authorize` job (write+ permission); | ||
| # this `if` only matches the /review command shape. | ||
| needs: ['authorize'] |
Collaborator
There was a problem hiding this comment.
[Suggestion] When the authorize job denies a /review command (non-write user or API error), this ack-review-request job is skipped because its if requires needs.authorize.outputs.should_review == 'true' and it lacks always(). No PR comment, reaction, or error message is posted — the user who typed /review gets zero feedback about why nothing happened.
Consider adding a lightweight fallback job that posts a brief comment when authorize denies:
ack-auth-denial:
needs: ['authorize']
if: |-
always() &&
needs.authorize.outputs.should_review == 'false' &&
(github.event_name == 'issue_comment' ||
github.event_name == 'pull_request_review_comment' ||
github.event_name == 'pull_request_review')
runs-on: 'ubuntu-latest'
steps:
- uses: peter-evans/create-or-update-comment@v4
with:
issue-number: ${{ github.event.issue.number || github.event.pull_request.number }}
body: >
This review command could not be processed.
Check the workflow run's authorize job for details.— qwen3.7-max via Qwen Code /review
yiliang114
enabled auto-merge (squash)
June 17, 2026 07:45
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
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
Replaces the trigger-authorization in the two legacy Qwen CI workflows (
qwen-code-pr-review.ymlandqwen-triage.yml). A newauthorizejob resolves the triggering principal — the PR author for automatic PR events, the commenter for/reviewand/triagecomment commands, and the requester forreview_requested— and verifies it has admin/maintain/write permission via the collaborators API. Downstream jobs gate on its output. This removes the per-pathauthor_associationchecks and the same-repo-only fork check that previously gated these workflows.Why it's needed
The triage job currently requires
head.repo == base, so every fork PR is skipped regardless of author — write users' fork PRs are never triaged. Review is gated onauthor_association, which is inconsistent for forks: write collaborators show asCOLLABORATORand already pass, but it does not reliably reflect actual permission. Moving both review and triage onto a real write-permission check lets fork PRs by trusted authors trigger consistently, and tightens the comment commands to actual write permission. External actors without write permission still cannot trigger the model-calling jobs.Reviewer Test Plan
How to verify
CI-only change to workflow gating. Verified locally:
actionlintreports only the two pre-existing findings (deploymentkey,ecs-qwenrunner label), YAML parses, andbash -npasses on the newauthorizeshell scripts. The authorization logic was traced end-to-end across every trigger path (automatic PR open/synchronize/reopened/ready_for_review,review_requested,/reviewand/triagecomments,workflow_dispatch, andissues).Evidence (Before & After)
N/A (non–user-visible CI change).
Tested on
Environment (optional)
N/A —
actionlint+ YAML parse +bash -n.Risk & Scope
authorizejob usesCI_BOT_PATbecause reading a user's collaborator permission requires write/maintain/admin access (theGITHUB_TOKENwithcontents: readdoes not have it). This is safe — the job runs no agent, checks out nothing, and processes no untrusted PR content; it only reads event metadata and calls one read API. The gate fails closed: any API error or non-write permission denies the run.review_requestedpath that used it was skipped in all recent runs), so confirm behavior after merge with one write-user trigger and one non-write-user trigger. The review agent's token model and prompt-injection posture are unchanged by this PR.authorize-review-requestwas renamed toauthorize. If branch protection lists the old job name as a required status check, update it to avoid blocking merges.Linked Issues
None (internal CI change).
中文说明
这个 PR 做了什么
替换两个旧版 Qwen CI 工作流(
qwen-code-pr-review.yml和qwen-triage.yml)的触发授权。新增一个authorizejob,解析触发主体——自动 PR 事件取 PR 作者,/review与/triage评论命令取评论者,review_requested取请求者——并通过 collaborators API 校验其是否具备 admin/maintain/write 权限,下游 job 据此门控。这移除了原先分散在各路径的author_association检查,以及 triage 里"仅同仓库分支"的 fork 检查。为什么需要
triage job 现在要求
head.repo == base,所以不管作者是谁,所有 fork PR 都被跳过——write 用户的 fork PR 从来不会被 triage。review 走的是author_association,对 fork 不一致:write 协作者显示为COLLABORATOR已经能过,但它不能可靠反映真实权限。把 review 和 triage 都挪到真正的 write 权限判断上,让受信任作者的 fork PR 一致触发,并把评论命令收紧到真正的 write 权限。无 write 权限的外部人仍无法触发调用模型的 job。Reviewer Test Plan
如何验证
仅改 CI 工作流门控。本地校验:
actionlint只剩两个原有报错(deploymentkey、ecs-qwenrunner label),YAML 可解析,新增的authorizeshell 脚本bash -n通过。授权逻辑已对每条触发路径端到端追踪(自动 PR open/synchronize/reopened/ready_for_review、review_requested、/review与/triage评论、workflow_dispatch、issues)。证据(Before & After)
N/A(非用户可见的 CI 改动)。
风险与范围
authorizejob 使用CI_BOT_PAT,因为读取用户的协作者权限需要 write/maintain/admin 访问(GITHUB_TOKEN的contents: read不具备)。这是安全的——该 job 不跑 agent、不 checkout、不处理任何不可信 PR 内容,只读事件元数据并调一个只读 API。门控 fail-closed:任何 API 错误或非 write 权限都拒绝运行。review_requested路径在近期所有 run 中都被跳过),合并后请用一个 write 用户触发 + 一个非 write 用户触发来确认行为。本 PR 不改变评审 agent 的 token 模型与 prompt 注入面。authorize-review-request已改名为authorize。若分支保护把旧 job 名设成了 required status check,请更新,以免卡住合并。关联 Issue
无(仓库内部 CI 改动)。