ci: request @clerk/team-mobile through the team review app

.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 {

.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),

.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,36 +73,42 @@ 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(', ')}`);
   }
   const message = await teamReviewRequest({
     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 {

.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, []);
     }
   });
 });

.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

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.