diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 6afd7be..03c076e 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -121,6 +121,15 @@ on: judgement of what needs a person. type: string default: '' + human-review-team: + description: >- + Team slug in the calling repository's org to request a review from + when a human review is needed, once per PR. The team's code review + auto-assignment decides who gets it. Empty disables the request. + Skipped without a github-token: the job token cannot request team + reviewers (GitHub answers 422, unable to resolve the team). + type: string + default: 'ux-team' tooling-ref: description: >- Ref of this repository to take the schema, rubric and scripts from. @@ -577,4 +586,6 @@ jobs: HEAD_SHA: ${{ github.event.pull_request.head.sha }} APPROVE_WHEN_CLEAN: ${{ inputs.approve-when-clean }} HUMAN_REVIEW_PATHS: ${{ inputs.human-review-paths }} + HUMAN_REVIEW_TEAM: ${{ inputs.human-review-team }} + HAS_GITHUB_TOKEN: ${{ secrets.github-token != '' }} run: node .claude-review/tooling/review/submit-verdict.mjs diff --git a/README.md b/README.md index 3b6b1b0..90c2212 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,7 @@ either, so granting it in a caller has no effect on the token the job runs with. | `extra-allowed-tools` | `''` | Comma-separated permission rules appended to the reviewer's `--allowedTools`, e.g. `Bash(go vet:*)`. Empty by default on purpose: anything that executes repo code runs PR-controlled code next to the job's write token, so each repo opts in as its own recorded decision | | `approve-when-clean` | `true` | Submit an APPROVE review when nothing blocks and no human review is needed. Set `false` to get a COMMENT saying it would have approved instead, to watch the calls before they count | | `human-review-paths` | `''` | Newline-separated globs, gitignore rules: `*`, `**`, `?`; a name with no slash (trailing one aside) matches at any depth, one with a slash is root-anchored, a directory match covers everything beneath it; no `!`, leading `/`, brackets or braces; `#` lines ignored. A PR touching a match always gets the needs-a-human COMMENT, whatever the reviewer decided | +| `human-review-team` | `ux-team` | Team in the calling repo's org asked for a review when a human review is needed, once per PR. Needs `github-token`; skipped with the job token. Empty disables it | | `tooling-ref` | `master` | Ref this repo's `review/` assets come from; see below | The secret is named, not inherited, because the repos call it different things @@ -284,7 +285,7 @@ cannot disagree with the check: | Result | Review submitted | Job | |---|---|---| | Anything at MEDIUM or above | REQUEST_CHANGES | fails | -| Only LOW or none, a human must look | COMMENT listing the reasons | passes, no approval | +| Only LOW or none, a human must look | COMMENT listing the reasons, and with a `github-token` a review request to `human-review-team` | passes, no approval | | Only LOW or none, `approve-when-clean` off | COMMENT "would approve" | passes | | Only LOW or none, `approve-when-clean` on | APPROVE | passes | | Only LOW or none, but GitHub refuses the APPROVE | COMMENT quoting the refusal | passes | @@ -325,6 +326,17 @@ review then stays until a person dismisses it. Making the review count is branch protection, per repo: +- **Turn on code review auto-assignment for the team** named by + `human-review-team`, in each org's team settings. The workflow requests the + team; auto-assignment is what swaps the team for one member. Without it the + whole team stays requested. The request is made once per PR and skipped if + any reviewer is already requested, so pushes do not reassign it. That + includes code owners: in a repo whose CODEOWNERS already names the team for + every file, GitHub requests it on every PR and this adds nothing. It needs + `github-token`, because the job token cannot request team reviewers; without + one the request is skipped with a log line. A fine-grained PAT may also need + the organisation Members read permission. A refused request is a warning, + never a change to the check. - **Require 1 approval** and mark `Automatic PR review` required. The workflow's approval satisfies the first, which is the whole mechanism and the whole risk: a PR the reviewer misjudges as routine merges with no person diff --git a/review/submit-verdict.mjs b/review/submit-verdict.mjs index 97e805a..0eb94a3 100644 --- a/review/submit-verdict.mjs +++ b/review/submit-verdict.mjs @@ -9,7 +9,8 @@ * `gh pr review`, so the review state on the PR cannot disagree with the check. * * any finding >= MEDIUM REQUEST_CHANGES, job fails - * clean, but a human must look COMMENT naming why, job passes + * clean, but a human must look COMMENT naming why, team review + * requested once per PR, job passes * clean, approve-when-clean off COMMENT "would approve", job passes * clean, approve-when-clean on APPROVE, job passes * clean, but GitHub refuses the APPROVE COMMENT saying so, job passes @@ -44,6 +45,9 @@ const repo = env('GITHUB_REPOSITORY'); const number = Number(env('PR_NUMBER')); const head = env('HEAD_SHA'); const approveWhenClean = env('APPROVE_WHEN_CLEAN') === 'true'; +const humanReviewTeam = env('HUMAN_REVIEW_TEAM'); +const hasGithubToken = env('HAS_GITHUB_TOKEN') === 'true'; +const REQUESTED_MARKER = ''; const humanReviewPaths = env('HUMAN_REVIEW_PATHS') .split('\n') .map((p) => p.trim()) @@ -83,6 +87,10 @@ const paginate = async (path) => { return all; }; +// May be filled before the verdict is posted; callers that must not see it exclude it by id. +let reviewsCache; +const listReviews = async () => (reviewsCache ??= await paginate(`/pulls/${number}/reviews`)); + /** The reviewer's terminal API error, when it died before producing output. */ const terminalApiError = () => { const file = env('EXECUTION_FILE'); @@ -226,7 +234,7 @@ const submit = async (event, lines) => { */ const dismissOwnStateReviews = async (own) => { try { - const reviews = await paginate(`/pulls/${number}/reviews`); + const reviews = await listReviews(); const stale = reviews.filter( (r) => ['CHANGES_REQUESTED', 'APPROVED'].includes(r.state) && @@ -247,6 +255,45 @@ const dismissOwnStateReviews = async (own) => { } }; +/** + * Asks the caller's org team for a review; the team's auto-assignment picks + * the person. Once per PR: skipped when any reviewer is already requested + * (CODEOWNERS included — after auto-assignment the team is often replaced by + * a user, so checking for the team alone would assign a second person), or + * when an earlier verdict recorded a request. Best effort. Returns the lines + * for the review body. + */ +const requestTeamReview = async () => { + if (!humanReviewTeam) return []; + const team = `${repo.split('/')[0]}/${humanReviewTeam}`; + if (!hasGithubToken) { + console.log(`Not requesting ${team}: the job token cannot request team reviewers. Pass a github-token.`); + return []; + } + try { + const requested = await api(`/pulls/${number}/requested_reviewers`); + if (requested.users?.length || requested.teams?.length) { + console.log('A reviewer is already requested; not requesting the team.'); + return []; + } + const reviews = await listReviews(); + if (reviews.some((r) => (r.body ?? '').includes(MARKER) && r.body.includes(REQUESTED_MARKER))) { + console.log(`${team} was requested on an earlier run; not requesting again.`); + return []; + } + await api(`/pulls/${number}/requested_reviewers`, { + method: 'POST', + body: JSON.stringify({ team_reviewers: [humanReviewTeam] }), + }); + console.log(`Requested a review from ${team}.`); + // Backticks, not an @-mention: a mention would notify the whole team. + return ['', `Requested a review from \`${team}\`.`, REQUESTED_MARKER]; + } catch (error) { + console.log(`::warning::could not request a review from ${team}: ${escapeData(error.message)}`); + return []; + } +}; + const approveHint = (error) => { if (!/HTTP 422/.test(error.message)) return; console.log( @@ -284,12 +331,14 @@ try { // On the flag, not the reason count: `required: true` with no reasons is // schema-valid and must not approve. if (humanReview.required || reasons.length > 0) { + const requestLines = await requestTeamReview(); const own = await submit('COMMENT', [ `**Needs a human review.** ${countLine}`, '', 'Nothing blocks, but this change is one a person should decide on:', '', ...(reasons.length ? reasons : ['The reviewer flagged this as needing a person but gave no reason.']).map((r) => `- ${r}`), + ...requestLines, ]); await dismissOwnStateReviews(own); console.log(`Human review required: ${reasons.length} reason(s).`);