diff --git a/.github/scripts/code-owner-approval.mjs b/.github/scripts/code-owner-approval.mjs index 94e7a14ff..7afe6a89c 100644 --- a/.github/scripts/code-owner-approval.mjs +++ b/.github/scripts/code-owner-approval.mjs @@ -107,7 +107,7 @@ async function main(env, { fetch = globalThis.fetch, out = process.stdout } = {} return pass; } -export { codeOwnerApproval, main, parseProtectedPaths, protectedFiles }; +export { codeOwnerApproval, main, parseProtectedPaths, protectedFiles, trusted }; if (process.argv[1] !== undefined && fs.realpathSync(process.argv[1]) === fileURLToPath(import.meta.url)) { try { diff --git a/.github/scripts/github.mjs b/.github/scripts/github.mjs index 64b8f51c6..202b0bb2e 100644 --- a/.github/scripts/github.mjs +++ b/.github/scripts/github.mjs @@ -1,6 +1,6 @@ function client({ fetch, api, token, repo }) { return async (path, { missing, method = 'GET', body } = {}) => { - const response = await fetch(`${api}/repos/${repo}${path}`, { + const response = await fetch(`${api}${repo === undefined ? '' : `/repos/${repo}`}${path}`, { method, headers: { accept: 'application/vnd.github+json', authorization: `Bearer ${token}`, 'x-github-api-version': '2022-11-28' }, body: body === undefined ? undefined : JSON.stringify(body), diff --git a/.github/scripts/team-review-request.mjs b/.github/scripts/team-review-request.mjs index 63404c779..9f0ccdf16 100644 --- a/.github/scripts/team-review-request.mjs +++ b/.github/scripts/team-review-request.mjs @@ -1,20 +1,13 @@ import fs from 'node:fs'; import { fileURLToPath } from 'node:url'; -import { codeOwnerApproval } from './code-owner-approval.mjs'; +import { codeOwnerApproval, trusted } from './code-owner-approval.mjs'; import { client, paged } from './github.mjs'; -const TEAM_FILE = new URL('../team-mobile', import.meta.url); +const TEAM = 'team-mobile'; const CODERABBIT = 'coderabbitai[bot]'; const HIGH_RISK = new Set(['high', 'critical']); -function parseTeam(text) { - return text - .split('\n') - .map(line => line.replace(/#.*$/, '').trim()) - .filter(line => line !== ''); -} - // CodeRabbit's summary comment carries its merge risk for the commit it last reviewed, and rewrites it after each review. function ratedHighRisk(comments, head) { return comments.some(comment => { @@ -28,6 +21,18 @@ function ratedHighRisk(comments, head) { }); } +function latestReviews(reviews) { + const latest = new Map(); + for (const review of reviews) { + if (review.user && review.state !== 'COMMENTED' && review.state !== 'PENDING') { + latest.set(review.user.login, review); + } else if (review.user && !latest.has(review.user.login)) { + latest.set(review.user.login, review); + } + } + return latest; +} + const PROTECTED_NOTICE = "This pull request changes files listed in `.github/protected-paths`, so it needs an approval from another Team Mobile member before it can merge. CodeRabbit's approval isn't enough for these files."; @@ -39,8 +44,10 @@ function protectedComment(files) { return [PROTECTED_NOTICE, '', ...shown].join('\n'); } -async function teamReviewRequest({ fetch, api, token, repo, number, team }) { +async function teamReviewRequest({ fetch, api, token, teamToken, repo, number }) { const get = client({ fetch, api, token, repo }); + // Only the app token can see org teams, so it reads the team and makes the review request. + const teamGet = client({ fetch, api, token: teamToken }); const pull = await get(`/pulls/${number}`); if (pull.state !== 'open' || pull.draft) { return 'The pull request is closed or a draft.'; @@ -66,21 +73,27 @@ async function teamReviewRequest({ fetch, api, token, repo, number, team }) { return `Requested nobody. ${check.message}`; } } - const asked = new Set([author, ...pull.requested_reviewers.map(user => user.login)]); - for (const review of await paged(get, `/pulls/${number}/reviews`)) { - asked.add(review.user?.login); + const members = (await paged(teamGet, `/orgs/${repo.split('/')[0]}/teams/${TEAM}/members`)).map(user => user.login); + // The team only grants write, and only members with maintain or admin can approve, so only they count as Team Mobile here. + const team = (await Promise.all(members.map(async login => ((await trusted(get, login)) ? login : null)))).filter(login => login !== null); + const pending = await teamGet(`/repos/${repo}/pulls/${number}/requested_reviewers`); + const asked = new Set([author, ...pending.users.map(user => user.login)]); + // A push dismisses earlier approvals, so a member whose approval was dismissed counts as not asked yet. + for (const [login, review] of latestReviews(await paged(get, `/pulls/${number}/reviews`))) { + if (review.state !== 'DISMISSED') { + asked.add(login); + } } - const reviewers = team.filter(login => !asked.has(login)); - if (reviewers.length === 0) { - return 'Every Team Mobile member is already requested or has reviewed.'; + if (pending.teams.some(requested => requested.slug === TEAM) || team.some(login => login !== author && asked.has(login))) { + return 'Team Mobile is already requested, or a member is requested or has reviewed.'; } - await get(`/pulls/${number}/requested_reviewers`, { method: 'POST', body: { reviewers } }); + await teamGet(`/repos/${repo}/pulls/${number}/requested_reviewers`, { method: 'POST', body: { team_reviewers: [TEAM] } }); const why = check.reason === 'coderabbit' || check.reason === 'protected' ? 'CodeRabbit rated this commit high or critical risk.' : check.message; - return `Requested ${reviewers.join(', ')}. ${why}`; + return `Requested @${repo.split('/')[0]}/${TEAM}. ${why}`; } -async function main(env, { fetch = globalThis.fetch, out = process.stdout, teamText = fs.readFileSync(TEAM_FILE, 'utf8') } = {}) { - const missing = ['GITHUB_REPOSITORY', 'GITHUB_TOKEN', 'PR_NUMBER'].filter(name => !env[name]); +async function main(env, { fetch = globalThis.fetch, out = process.stdout } = {}) { + const missing = ['GITHUB_REPOSITORY', 'GITHUB_TOKEN', 'TEAM_REVIEW_TOKEN', 'PR_NUMBER'].filter(name => !env[name]); if (missing.length > 0) { throw new Error(`missing ${missing.join(', ')}`); } @@ -88,14 +101,14 @@ async function main(env, { fetch = globalThis.fetch, out = process.stdout, teamT fetch, api: env.GITHUB_API_URL || 'https://api.github.com', token: env.GITHUB_TOKEN, + teamToken: env.TEAM_REVIEW_TOKEN, repo: env.GITHUB_REPOSITORY, number: env.PR_NUMBER, - team: parseTeam(teamText), }); out.write(`${message}\n`); } -export { main, parseTeam, ratedHighRisk }; +export { main, ratedHighRisk }; if (process.argv[1] !== undefined && fs.realpathSync(process.argv[1]) === fileURLToPath(import.meta.url)) { try { diff --git a/.github/scripts/team-review-request.test.mjs b/.github/scripts/team-review-request.test.mjs index 23cbb8dad..1727fbb74 100644 --- a/.github/scripts/team-review-request.test.mjs +++ b/.github/scripts/team-review-request.test.mjs @@ -1,14 +1,13 @@ import assert from 'node:assert/strict'; -import fs from 'node:fs'; import { describe, it } from 'node:test'; -import { main, parseTeam, ratedHighRisk } from './team-review-request.mjs'; +import { main, ratedHighRisk } from './team-review-request.mjs'; const REPO = 'clerk/clerk-ios'; const API = 'https://api.github.test'; const HEAD = 'c14cacc2763d2dfc271d9c286b09376a90c5b9ff'; -const TEAM = '# Team Mobile\nmikepitre\nseanperez29\nswolfand\nwobsoriano\n'; -const ROLES = { mikepitre: 'admin', seanperez29: 'admin', swolfand: 'maintain', wobsoriano: 'maintain', 'outside-dev': 'write' }; +const TEAM = ['swolfand', 'mikepitre', 'wobsoriano', 'seanperez29', 'team-newcomer']; +const ROLES = { mikepitre: 'admin', seanperez29: 'admin', swolfand: 'maintain', wobsoriano: 'maintain', 'outside-dev': 'write', 'team-newcomer': 'write' }; // The shape CodeRabbit writes into its summary comment. const riskComment = (level, commit = HEAD) => ({ @@ -17,19 +16,31 @@ const riskComment = (level, commit = HEAD) => ({ }); const review = (login, state) => ({ user: { login }, state }); -function fakeGitHub({ author = 'outside-dev', draft = false, state = 'open', files = [{ filename: 'Sources/ClerkKit/Clerk.swift' }], pending = [], reviews = [], comments = [] }) { +function fakeGitHub({ author = 'outside-dev', draft = false, state = 'open', files = [{ filename: 'Sources/ClerkKit/Clerk.swift' }], pending = [], pendingTeams = [], reviews = [], comments = [] }) { const requested = []; + const teams = []; const posted = []; const edited = []; const reply = body => ({ ok: true, status: 200, json: async () => body, text: async () => JSON.stringify(body) }); const notFound = { ok: false, status: 404, json: async () => ({}), text: async () => 'Not Found' }; - const fetch = async (url, { method = 'GET', body }) => { + const fetch = async (url, { method = 'GET', headers, body }) => { const { pathname, searchParams } = new URL(url); const path = pathname.replace(`/repos/${REPO}`, ''); if (method === 'POST' && path === '/pulls/7/requested_reviewers') { - requested.push(...JSON.parse(body).reviewers); + assert.equal(headers.authorization, 'Bearer app-token'); + const { reviewers = [], team_reviewers = [] } = JSON.parse(body); + requested.push(...reviewers); + teams.push(...team_reviewers); return reply({}); } + if (method === 'GET' && path === '/pulls/7/requested_reviewers') { + assert.equal(headers.authorization, 'Bearer app-token'); + return reply({ users: pending.map(login => ({ login })), teams: pendingTeams.map(slug => ({ slug })) }); + } + if (path === '/orgs/clerk/teams/team-mobile/members') { + assert.equal(headers.authorization, 'Bearer app-token'); + return reply(Number(searchParams.get('page')) === 1 ? TEAM.map(login => ({ login })) : []); + } if (method === 'POST' && path === '/issues/7/comments') { posted.push(JSON.parse(body).body); return reply({}); @@ -40,7 +51,7 @@ function fakeGitHub({ author = 'outside-dev', draft = false, state = 'open', fil return reply({}); } if (path === '/pulls/7') { - return reply({ state, draft, user: { login: author }, head: { sha: HEAD }, base: { ref: 'main' }, changed_files: files.length, requested_reviewers: pending.map(login => ({ login })) }); + return reply({ state, draft, user: { login: author }, head: { sha: HEAD }, base: { ref: 'main' }, changed_files: files.length }); } if (path === '/contents/.github/protected-paths') { return reply({ content: Buffer.from('/.github/workflows/\n').toString('base64') }); @@ -56,17 +67,17 @@ function fakeGitHub({ author = 'outside-dev', draft = false, state = 'open', fil } throw new Error(`unexpected ${method} ${path}`); }; - return { fetch, requested, posted, edited }; + return { fetch, requested, teams, posted, edited }; } async function run(github) { - const { fetch, requested, posted, edited } = fakeGitHub(github); + const { fetch, requested, teams, posted, edited } = fakeGitHub(github); const lines = []; await main( - { GITHUB_API_URL: API, GITHUB_REPOSITORY: REPO, GITHUB_TOKEN: 'test-token', PR_NUMBER: '7' }, - { fetch, out: { write: line => lines.push(line) }, teamText: TEAM }, + { GITHUB_API_URL: API, GITHUB_REPOSITORY: REPO, GITHUB_TOKEN: 'test-token', TEAM_REVIEW_TOKEN: 'app-token', PR_NUMBER: '7' }, + { fetch, out: { write: line => lines.push(line) } }, ); - return { requested, posted, edited, output: lines.join('') }; + return { requested, teams, posted, edited, output: lines.join('') }; } describe('ratedHighRisk', () => { @@ -83,29 +94,25 @@ describe('ratedHighRisk', () => { }); describe('team review request', () => { - it('lists GitHub logins in the committed team file', () => { - const team = parseTeam(fs.readFileSync(new URL('../team-mobile', import.meta.url), 'utf8')); - assert.ok(team.length > 0); - assert.ok(team.every(login => /^[A-Za-z0-9-]+$/.test(login))); - }); - it('requests nobody for a Team Mobile author on ordinary files while CodeRabbit reviews', async () => { const { requested, output } = await run({ author: 'swolfand' }); assert.deepEqual(requested, []); assert.match(output, /Waiting on CodeRabbit/); }); - it('requests the other members when CodeRabbit rates a Team Mobile author\'s commit high risk', async () => { - const { requested, posted, output } = await run({ author: 'swolfand', comments: [riskComment('High')] }); - assert.deepEqual(requested, ['mikepitre', 'seanperez29', 'wobsoriano']); + it('requests the team when CodeRabbit rates a Team Mobile author\'s commit high risk', async () => { + const { requested, teams, posted, output } = await run({ author: 'swolfand', comments: [riskComment('High')] }); + assert.deepEqual(requested, []); + assert.deepEqual(teams, ['team-mobile']); assert.deepEqual(posted, []); - assert.match(output, /CodeRabbit rated this commit high or critical risk/); + assert.match(output, /Requested @clerk\/team-mobile\. CodeRabbit rated this commit high or critical risk/); }); it('comments with the protected files and requests nobody when a Team Mobile author changes a protected file', async () => { const files = [{ filename: '.github/workflows/pr-ci.yml' }, { filename: 'Sources/ClerkKit/Clerk.swift' }]; - const { requested, posted } = await run({ author: 'mikepitre', files }); + const { requested, teams, posted } = await run({ author: 'mikepitre', files }); assert.deepEqual(requested, []); + assert.deepEqual(teams, []); assert.equal(posted.length, 1); assert.match(posted[0], /changes files listed in `\.github\/protected-paths`/); assert.match(posted[0], /^- `\.github\/workflows\/pr-ci\.yml`$/m); @@ -132,35 +139,57 @@ describe('team review request', () => { assert.doesNotMatch(again.edited[0].body, /pr-ci\.yml/); }); - it('requests the other members when CodeRabbit rates a protected change high risk', async () => { - const { requested, posted, output } = await run({ author: 'mikepitre', files: [{ filename: '.github/workflows/pr-ci.yml' }], comments: [riskComment('High')] }); - assert.deepEqual(requested, ['seanperez29', 'swolfand', 'wobsoriano']); + it('requests the team when CodeRabbit rates a protected change high risk', async () => { + const { teams, posted, output } = await run({ author: 'mikepitre', files: [{ filename: '.github/workflows/pr-ci.yml' }], comments: [riskComment('High')] }); + assert.deepEqual(teams, ['team-mobile']); assert.equal(posted.length, 1); assert.match(output, /CodeRabbit rated this commit high or critical risk/); }); it('requests Team Mobile for an author with write access, a fork contributor and a bot', async () => { for (const author of ['outside-dev', 'fork-contributor', 'dependabot[bot]']) { - const { requested, posted } = await run({ author }); - assert.deepEqual(requested, ['mikepitre', 'seanperez29', 'swolfand', 'wobsoriano']); + const { requested, teams, posted } = await run({ author }); + assert.deepEqual(requested, []); + assert.deepEqual(teams, ['team-mobile']); assert.deepEqual(posted, []); } }); - it('skips members who are already requested or have reviewed', async () => { - const { requested } = await run({ pending: ['mikepitre'], reviews: [review('swolfand', 'COMMENTED')] }); - assert.deepEqual(requested, ['seanperez29', 'wobsoriano']); + it('requests nobody when the team or a member is already requested, or a member has reviewed', async () => { + for (const github of [{ pendingTeams: ['team-mobile'] }, { pending: ['mikepitre'] }, { reviews: [review('swolfand', 'COMMENTED')] }]) { + const { requested, teams, output } = await run(github); + assert.deepEqual(requested, []); + assert.deepEqual(teams, []); + assert.match(output, /Team Mobile is already requested/); + } + }); + + it('still requests the team after a review from a member who has only write access', async () => { + const { teams } = await run({ reviews: [review('team-newcomer', 'COMMENTED')] }); + assert.deepEqual(teams, ['team-mobile']); + }); + + it('requests the team again when a push dismissed the only member review', async () => { + const { teams } = await run({ reviews: [review('wobsoriano', 'APPROVED'), review('wobsoriano', 'DISMISSED')] }); + assert.deepEqual(teams, ['team-mobile']); + }); + + it('requests nobody while a member who requested changes is still reviewing', async () => { + const { teams } = await run({ reviews: [review('swolfand', 'CHANGES_REQUESTED'), review('swolfand', 'COMMENTED')] }); + assert.deepEqual(teams, []); }); it('requests nobody once Team Mobile approved', async () => { - const { requested } = await run({ reviews: [review('wobsoriano', 'APPROVED')] }); + const { requested, teams } = await run({ reviews: [review('wobsoriano', 'APPROVED')] }); assert.deepEqual(requested, []); + assert.deepEqual(teams, []); }); it('skips drafts and closed pull requests', async () => { for (const github of [{ draft: true }, { state: 'closed' }]) { - const { requested } = await run(github); + const { requested, teams } = await run(github); assert.deepEqual(requested, []); + assert.deepEqual(teams, []); } }); }); diff --git a/.github/team-mobile b/.github/team-mobile deleted file mode 100644 index 08806cc94..000000000 --- a/.github/team-mobile +++ /dev/null @@ -1,6 +0,0 @@ -# GitHub logins that Team review request asks to review a pull request from someone without maintain or admin access. -# Keep this in step with the @clerk/team-mobile team. -mikepitre -seanperez29 -swolfand -wobsoriano diff --git a/.github/workflows/team-review-request.yml b/.github/workflows/team-review-request.yml index f7ec9bd16..0f8ca13dd 100644 --- a/.github/workflows/team-review-request.yml +++ b/.github/workflows/team-review-request.yml @@ -26,17 +26,26 @@ jobs: issues: read pull-requests: write steps: - - name: Checkout the scripts and team list from main + - name: Checkout the scripts from main uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ github.event.repository.default_branch }} sparse-checkout: | .github/scripts - .github/team-mobile sparse-checkout-cone-mode: false persist-credentials: false + # The Actions token can't see org teams, so this app reads @clerk/team-mobile and requests it. + - name: Mint the team review token + id: team-token + uses: actions/create-github-app-token@64ab729a6294000f5aa4a4bc0b73d1f32bc45510 # v3.3.0 + with: + client-id: ${{ vars.TEAM_REVIEW_APP_CLIENT_ID }} + private-key: ${{ secrets.TEAM_REVIEW_APP_PRIVATE_KEY }} + permission-members: read + permission-pull-requests: write - name: Request Team Mobile when the PR needs a Team Mobile approval env: GITHUB_TOKEN: ${{ github.token }} + TEAM_REVIEW_TOKEN: ${{ steps.team-token.outputs.token }} PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }} run: node .github/scripts/team-review-request.mjs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 29dcc4405..4527f4da5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -138,11 +138,11 @@ Every PR also needs the `Code owner approval` check. Team Mobile means anyone wi or admin access to the repository. A Team Mobile author needs only CodeRabbit's approval, unless the PR changes a path in `.github/protected-paths` or CodeRabbit rates it high or critical risk. Then another Team Mobile member has to approve. Anyone else needs an -approval from Team Mobile. The `Team review request` workflow requests the members listed -in `.github/team-mobile` when a PR needs one, so keep that list in step with the team. +approval from Team Mobile. The `Team review request` workflow requests `@clerk/team-mobile` +when a PR needs one. When a Team Mobile author changes a protected path, it comments on the PR with the protected files and requests nobody, so the author asks a member to review. It still requests the -members if CodeRabbit rates that PR high or critical. GitHub counts every run of the check +team if CodeRabbit rates that PR high or critical. GitHub counts every run of the check on a commit, so when a review makes the check pass, that run also reruns the ones that failed before the review. A fork PR's token is read-only and can't rerun them, so a maintainer reruns the failed runs by hand.