This is an automated email from the ASF dual-hosted git repository. hubcio pushed a commit to branch feat/pr-triage-bot in repository https://gitbox.apache.org/repos/asf/iggy.git
commit 0a3eadb168ebe53b51fff5faf2c7323e68494e8b Author: Hubert Gruszecki <[email protected]> AuthorDate: Sat May 9 13:15:07 2026 +0200 feat(ci): label PR review state via slash commands and lifecycle Reviewers cannot tell at a glance which open PRs are still in their queue. Iggy's .asf.yaml requires 2 approvals with stale-dismiss, so each push wipes prior approvals - the review backlog grows opaque fast as PR volume rises. Adopt rust-lang/triagebot's S-waiting-on-{review,author} pattern via a single GitHub Actions workflow. Comment commands /ready, /author, and /request-review @user move the labels explicitly; PR lifecycle events (open, ready_for_review, converted_to_draft, closed) keep them in sync without manual upkeep. Filter the queue with `is:open is:pr label:S-waiting-on-review`. The auth gate is author_association in {COLLABORATOR, OWNER}, which matches @apache/iggy-committers. MEMBER is excluded deliberately - it would admit any unrelated apache podling member. issue_comment.created and pull_request_target are the only triggers; the workflow never checks out a ref or executes PR-supplied code, only calls the REST API via actions/github-script. This avoids the pwn-request RCE class and stays inside the default GITHUB_TOKEN scope - no PAT, no INFRA Jira ticket, no external host. CODEOWNERS gains a `* @apache/iggy-committers` wildcard so reviewer auto-request fires on every PR, not just .github/** changes. --- .github/CODEOWNERS | 2 +- .github/workflows/_detect.yml | 2 +- .github/workflows/pr-triage.yml | 351 ++++++++++++++++++++++++++++++++++++++++ .github/workflows/pre-merge.yml | 2 +- .github/workflows/publish.yml | 2 +- CONTRIBUTING.md | 107 ++++++++++++ 6 files changed, 462 insertions(+), 4 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 35cc5645d..1720bb1f4 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1 +1 @@ -.github/** @apache/iggy-committers +* @apache/iggy-committers diff --git a/.github/workflows/_detect.yml b/.github/workflows/_detect.yml index ff11fe903..41a37ecb8 100644 --- a/.github/workflows/_detect.yml +++ b/.github/workflows/_detect.yml @@ -91,7 +91,7 @@ jobs: - name: Build matrices id: mk - uses: actions/github-script@v8 + uses: actions/github-script@v9 with: script: | const componentsJson = `${{ steps.config.outputs.components }}`; diff --git a/.github/workflows/pr-triage.yml b/.github/workflows/pr-triage.yml new file mode 100644 index 000000000..2229ae791 --- /dev/null +++ b/.github/workflows/pr-triage.yml @@ -0,0 +1,351 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: PR Triage + +# Comment-driven PR triage and lifecycle labels. Inspired by +# rust-lang/triagebot UX. +# +# Comment commands (parsed line-by-line in PR comments): +# /request-review @u [@u2 ...] — request review from one or more +# @users or @org/teams; the command may +# also repeat across lines +# /ready — flip state to S-waiting-on-review +# /author — flip state to S-waiting-on-author +# +# Auth gate: +# /request-review, /ready -> repo COLLABORATOR/OWNER, or PR author +# /author -> repo COLLABORATOR/OWNER only +# +# Lifecycle (pull_request_target): +# opened (non-draft) -> add S-waiting-on-review (only if no S-* present) +# ready_for_review -> add S-waiting-on-review (only if no S-* present) +# converted_to_draft -> remove both S-* labels +# closed (merged or not) -> remove both S-* labels +# +# SECURITY: +# - issue_comment.created and pull_request_target are the ONLY triggers. +# - No actions/checkout of any ref. PR-controlled code is never executed. +# - The default GITHUB_TOKEN is never written to outputs, env files, or +# passed to user-supplied programs. The workflow only calls the GitHub +# REST API via actions/github-script. +# - pull_request_target is used so fork-PR labels can be applied with the +# base-repo's write token. This is safe because we never run fork code: +# no checkout, no exec of PR contents, no token export. See +# https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ + +on: + issue_comment: + types: [created] + pull_request_target: + types: [opened, ready_for_review, converted_to_draft, closed] + +permissions: + pull-requests: write + issues: write + contents: read + +concurrency: + # cancel-in-progress: false keeps the active run, but GH still replaces + # any pending run with the latest arrival on burst commands. Net effect + # under N rapid commands: the active run plus the latest arrival land, + # everything queued in between is dropped. Acceptable because /ready + # and /author are idempotent (state is reflected in the final label) + # and /request-review is the only non-idempotent command; a 3-command + # /request-review burst in <1s lands at most 2 reviewers, larger bursts + # lose proportionally more. Pending-queue retention key (queue:) is not + # yet in the GH Actions schema; revisit when available. + group: pr-triage-${{ github.event.pull_request.number || github.event.issue.number }} + cancel-in-progress: false + +jobs: + triage: + if: | + (github.event_name == 'issue_comment' && github.event.issue.pull_request != null) + || github.event_name == 'pull_request_target' + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Dispatch + uses: actions/github-script@v9 + env: + COMMENT_BODY: ${{ github.event.comment.body }} + COMMENT_AUTHOR: ${{ github.event.comment.user.login }} + COMMENT_ASSOC: ${{ github.event.comment.author_association }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }} + with: + script: | + const LABEL_REVIEW = 'S-waiting-on-review'; + const LABEL_AUTHOR = 'S-waiting-on-author'; + const COMMITTER_ASSOCS = new Set(['COLLABORATOR', 'OWNER']); + const prNumber = Number(process.env.PR_NUMBER); + + const removeLabelIfPresent = async (name) => { + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name, + }); + } catch (e) { + if (e.status !== 404) throw e; + } + }; + + const setLabels = async (add, remove) => { + // Invariant: "neither S-* label" is recoverable (next command + // sets the correct one); "both S-* labels" is NOT (lifecycle + // hasState gate skips when any S-* is present, so a stuck + // both-labels state never gets cleaned up). Remove-first is + // strictly safer than add-first under crash mid-op. + // + // Transient 5xx on the add half would leave the PR in a + // label-less limbo that lifecycle hooks don't re-enter (they + // only fire on opened/ready_for_review). On add failure + // re-add the removed label to restore the prior single-label + // state, then surface the original error. + await removeLabelIfPresent(remove); + try { + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: [add], + }); + } catch (e) { + try { + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: [remove], + }); + } catch (rollbackErr) { + core.warning(`setLabels rollback failed: ${rollbackErr.message}`); + } + throw e; + } + }; + + // ----- pull_request_target lifecycle ----- + if (context.eventName === 'pull_request_target') { + const action = context.payload.action; + const pr = context.payload.pull_request; + + core.info(`lifecycle event=${action} draft=${pr.draft}`); + + if (action === 'closed' || action === 'converted_to_draft') { + // Always attempt the DELETE: pr.labels is the webhook + // snapshot and can disagree with current state via + // out-of-band edits or in-flight comment-handler races. + // removeLabelIfPresent swallows 404 so two no-op calls on + // a label-less PR are cheap. + await removeLabelIfPresent(LABEL_REVIEW); + await removeLabelIfPresent(LABEL_AUTHOR); + core.info(`lifecycle: cleared S-* labels (${action})`); + return; + } + + if (action === 'opened' || action === 'ready_for_review') { + if (pr.draft) { + core.info('lifecycle: draft PR, no label'); + return; + } + // Read live labels rather than the webhook-frozen + // pr.labels — a comment-driven /author or /ready may have + // landed between the lifecycle event and this run. + const live = await github.rest.issues.listLabelsOnIssue({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + per_page: 100, + }); + const liveNames = live.data.map(l => l.name); + const hasState = liveNames.includes(LABEL_REVIEW) + || liveNames.includes(LABEL_AUTHOR); + core.info(`lifecycle: has_state=${hasState}`); + if (hasState) { + core.info('lifecycle: S-* label already present, no change'); + return; + } + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: [LABEL_REVIEW], + }); + core.info(`lifecycle: added ${LABEL_REVIEW}`); + return; + } + + core.info(`lifecycle: unhandled action ${action}`); + return; + } + + // ----- issue_comment commands ----- + const body = process.env.COMMENT_BODY || ''; + + // Skip everything if no command line is present. Avoids a + // pulls.get on every drive-by PR comment. Leading whitespace + // is tolerated so that " /ready" matches the same way the + // line-by-line dispatch (which trims) does. The trailing + // `(?:\s|$)` rejects suffixed prose like `/ready-to-merge` — + // `\b` fires at hyphen/slash and would silently flip state. + const COMMAND_RE = /^[ \t]*\/(request-review|ready|author)(?:\s|$)/m; + if (!COMMAND_RE.test(body)) { + core.info('no command in body, skipping'); + return; + } + + const commentAuthor = process.env.COMMENT_AUTHOR; + const commentAssoc = process.env.COMMENT_ASSOC; + // Bot-suffixed accounts (e.g. dependabot[bot]) cannot drive + // commands as committer or PR author — would let bots + // self-flip review state. + const isBot = /\[bot\]$/.test(commentAuthor); + const isCommitter = COMMITTER_ASSOCS.has(commentAssoc) && !isBot; + + // Live state read. payload.issue.state is the webhook-frozen + // value at delivery; comment-while-open + close-arriving-later + // would otherwise re-label a now-closed PR. Run on both + // committer and non-committer paths — correctness over the + // single API call saved on the committer fast-path. + let prData; + try { + prData = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }); + } catch (e) { + if (e.status === 404) { + core.info('PR not found (deleted or transferred), skipping'); + return; + } + throw e; + } + + if (prData.data.state === 'closed') { + core.info('PR is closed, skipping'); + return; + } + + const prAuthor = prData.data.user.login; + const isPrAuthor = commentAuthor === prAuthor && !isBot; + + core.info(`comment_author=${commentAuthor} assoc=${commentAssoc} ` + + `committer=${isCommitter} pr_author=${isPrAuthor} bot=${isBot}`); + + const lines = body.split(/\r?\n/).map(l => l.trim()); + let sawReassign = false; + let sawReady = false; + let sawAuthor = false; + + // /request-review handles accumulate across every matching line + // (and every handle on a line); the requestReviewers call is + // deferred until after the scan so N handles cost one API call, + // not N. Sets dedupe handles repeated within the same comment. + const reviewers = new Set(); + const teamReviewers = new Set(); + + // GH username: alnum + hyphen, no leading/trailing hyphen. + // Optional team slug: '/' then alnum + underscore + hyphen. + // Fully anchored so a malformed handle (empty slug @foo/, extra + // segment @foo/bar/baz) is rejected per-token, not silently + // truncated. + const HANDLE_RE = /^@([A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?(?:\/[A-Za-z0-9_-]+)?)$/; + + for (const line of lines) { + // Unlike /ready and /author, /request-review may legitimately + // repeat across lines, so no first-match-wins guard and no + // early loop break. + const rr = line.match(/^\/request-review(?:\s+(.*))?$/); + if (rr) { + sawReassign = true; + if (!(isCommitter || isPrAuthor)) { + core.info(`/request-review: ignored, ${commentAuthor} lacks permission`); + continue; + } + const rest = (rr[1] || '').trim(); + if (!rest) { + core.info('/request-review: no reviewer specified'); + continue; + } + for (const token of rest.split(/\s+/)) { + const hm = token.match(HANDLE_RE); + if (!hm) { + core.warning(`/request-review: ignoring malformed handle "${token}"`); + continue; + } + const handle = hm[1]; + // Team slugs go through team_reviewers as bare slug, no org. + if (handle.includes('/')) { + teamReviewers.add(handle.split('/')[1]); + } else { + reviewers.add(handle); + } + } + continue; + } + if (!sawReady && /^\/ready(?:\s|$)/.test(line)) { + sawReady = true; + if (!(isCommitter || isPrAuthor)) { + core.info(`/ready: ignored, ${commentAuthor} lacks permission`); + } else { + await setLabels(LABEL_REVIEW, LABEL_AUTHOR); + core.info(`/ready: ${LABEL_REVIEW} <- ${LABEL_AUTHOR}`); + } + continue; + } + if (!sawAuthor && /^\/author(?:\s|$)/.test(line)) { + sawAuthor = true; + if (!isCommitter) { + core.info(`/author: ignored, ${commentAuthor} not a committer`); + } else { + await setLabels(LABEL_AUTHOR, LABEL_REVIEW); + core.info(`/author: ${LABEL_AUTHOR} <- ${LABEL_REVIEW}`); + } + continue; + } + } + + if (reviewers.size > 0 || teamReviewers.size > 0) { + const params = { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }; + if (reviewers.size > 0) params.reviewers = [...reviewers]; + if (teamReviewers.size > 0) params.team_reviewers = [...teamReviewers]; + const requested = [...reviewers, ...teamReviewers].join(', '); + try { + await github.rest.pulls.requestReviewers(params); + core.info(`/request-review: requested ${requested}`); + } catch (e) { + // GitHub rejects the whole batch on a single bad handle + // (non-collaborator, unknown team) with one 422. Per-handle + // calls would isolate the failure but multiply API calls + // for a rare path; the run log names the rejected set and + // the commenter re-posts. + core.warning(`/request-review: failed to request [${requested}]: ${e.message}`); + } + } + + if (!sawReassign && !sawReady && !sawAuthor) { + core.info('no command matched'); + } diff --git a/.github/workflows/pre-merge.yml b/.github/workflows/pre-merge.yml index 0cac3a234..1e9d5312d 100644 --- a/.github/workflows/pre-merge.yml +++ b/.github/workflows/pre-merge.yml @@ -199,7 +199,7 @@ jobs: steps: - name: Get job execution times id: times - uses: actions/github-script@v8 + uses: actions/github-script@v9 with: script: | const jobs = await github.rest.actions.listJobsForWorkflowRun({ diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index c8a29a088..0c246145e 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -305,7 +305,7 @@ jobs: - name: Build matrix from inputs id: mk - uses: actions/github-script@v8 + uses: actions/github-script@v9 with: script: | const componentsB64 = '${{ steps.cfg.outputs.components_b64 }}'; diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a10060c65..0bb53a1d5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -102,6 +102,113 @@ chore(integration): remove streaming tests superseded by API-level coverage Keep subject under 72 chars. Use body for details if needed. +## PR Triage Commands + +You can move a PR around the review queue by posting a slash command in the +PR conversation. The pattern is similar to `rust-lang/triagebot`. The +machinery lives in [`./.github/workflows/pr-triage.yml`](./.github/workflows/pr-triage.yml) +if you want to peek. + +### Commands + +| Command | Who can use it | What it does | +| --- | --- | --- | +| `/request-review @user-or-team ...` | the PR author or a maintainer | Requests review from one or more `@user` or `@org/team` handles | +| `/ready` | the PR author or a maintainer | Marks the PR `S-waiting-on-review` | +| `/author` | a maintainer | Marks the PR `S-waiting-on-author` | + +A "maintainer" here means someone with write access to apache/iggy +(in practice, the `@apache/iggy-committers` team). Automated comments from +bots like Dependabot do not run commands. + +Each command has to start its own line. Leading whitespace is fine, but +prose like `please /ready` will not match. You can put more than one command +in a single comment: `/request-review` plus `/ready`, or `/request-review` +plus `/author`. `/request-review` may carry several handles on one line and +may also repeat across lines; all of them are collected and requested +together. For `/ready` and `/author`, the last one wins, so `/ready` then +`/author` in the same comment ends up as `S-waiting-on-author`. + +### Typical flow + +1. You open a PR. CODEOWNERS pings `@apache/iggy-committers` automatically. +2. A maintainer reviews. If they want changes, they comment `/author` and + the PR moves to your queue. +3. You push fixes, then comment `/ready`. The PR moves back to the review + queue. +4. Either side can comment `/request-review @somebody` to pull in a + specific person or team. + +To find PRs waiting on review, filter with +`is:open is:pr label:S-waiting-on-review` on the Pulls tab. + +### Lifecycle automation + +Some labels are managed for you based on what happens to the PR: + +| When | What happens | +| --- | --- | +| You open a PR (not a draft) | Gets `S-waiting-on-review`, unless an `S-*` label is already set | +| You mark a draft "Ready for review" | Gets `S-waiting-on-review`, unless an `S-*` label is already set | +| You convert the PR back to a draft | Both `S-*` labels are removed | +| The PR is closed or merged | Both `S-*` labels are removed | + +Reopening a PR does not re-label it. Drop a `/ready` or `/author` comment +to put it back in a queue. + +Drafts are skipped by the automatic labelling, but `/ready` and `/author` +still work on drafts if you want to signal intent before clicking "Ready +for review". + +### Tips + +- **One comment per command burst.** To request several reviewers at once, + list them on a single `/request-review` line (or several lines) in one + comment. Posting separate comments back-to-back can lose the middle one + due to how CI runs are scheduled. +- **Edits don't re-trigger.** If you typo a command and edit the comment, it + will not run. Post a new comment instead. +- **Use the main conversation, not inline review replies.** Commands posted + as replies on a specific line of the diff are ignored. Post them in the + PR's main comment thread. +- **Suffixes don't match.** `/ready-to-merge` or `/readyish` will not flip + state - only `/ready` followed by a space or end-of-line counts. + +### When something goes wrong + +The workflow never comments back. If your command didn't seem to do +anything, open the PR's "Checks" or "Actions" tab and look at the +`PR Triage` run for the comment you posted. The run log says exactly what +it saw and why (no permission, unknown user, etc.). + +### Examples + +Mark your PR ready after addressing feedback: + +```text +/ready +``` + +Ask the author to take another look: + +```text +/author +``` + +Request a specific reviewer and mark ready in one comment: + +```text +/request-review @somebody +/ready +``` + +Request several reviewers in one go (do this instead of posting separate +comments). They can share one line, span multiple lines, or both: + +```text +/request-review @alice @bob @apache/iggy-committers +``` + ## Close Policy PRs may be closed if:
