ci: require a Team Mobile approval unless a member wrote a low-risk change

.github/protected-paths

@@ -1,4 +1,4 @@
-# A change to these paths needs an approval from someone with maintain or admin access, unless one of them wrote it.
+# A change to these paths needs an approval from a Team Mobile member (maintain or admin access) other than the author.
 # The `Code owner approval` check enforces this. Patterns follow CODEOWNERS syntax, without owners.
 
 /.github/protected-paths

.github/scripts/code-owner-approval.mjs

@@ -5,6 +5,7 @@ import { client, paged } from './github.mjs';
 
 const TRUSTED_ROLES = new Set(['admin', 'maintain']);
 const PROTECTED_PATHS = '.github/protected-paths';
+const CODERABBIT = 'coderabbitai[bot]';
 
 const GLOB = { '**/': '(?:.*/)?', '/**': '(?:/.*)?', '**': '.*', '*': '[^/]*', '?': '[^/]' };
 
@@ -50,32 +51,41 @@ async function trusted(get, login) {
   return TRUSTED_ROLES.has(role);
 }
 
+// reason says why the check failed: 'author', 'files', 'protected' or 'coderabbit'.
 async function codeOwnerApproval({ fetch, api, token, repo, number, author, baseRef }) {
   const get = client({ fetch, api, token, repo });
+  const pull = await get(`/pulls/${number}`);
+  const reviews = await paged(get, `/pulls/${number}/reviews`);
+  const approved = approvers(reviews, author);
+  for (const login of approved) {
+    if (await trusted(get, login)) {
+      return { pass: true, message: `${login} approved, and has maintain or admin access.` };
+    }
+  }
+  if (!(await trusted(get, author))) {
+    return { pass: false, reason: 'author', message: `${author} doesn't have maintain or admin access, so this needs an approval from Team Mobile.` };
+  }
   const list = await get(`/contents/${PROTECTED_PATHS}?ref=${encodeURIComponent(baseRef)}`, { missing: null });
   const patterns = list === null ? [] : parseProtectedPaths(Buffer.from(list.content, 'base64').toString('utf8'));
   const files = await paged(get, `/pulls/${number}/files`);
-  const { changed_files: changed } = await get(`/pulls/${number}`);
+  const changed = pull.changed_files;
   if (files.length < changed) {
-    return { pass: false, message: `GitHub listed ${files.length} of the ${changed} changed files, so this check can't tell which are protected. Split the pull request.` };
+    return { pass: false, reason: 'files', message: `GitHub listed ${files.length} of the ${changed} changed files, so this check can't tell which are protected. Split the pull request.` };
   }
   const owned = protectedFiles(patterns, [...new Set(files.flatMap(file => [file.filename, file.previous_filename].filter(Boolean)))]);
-  if (owned.length === 0) {
-    return { pass: true, message: `No file listed in ${PROTECTED_PATHS} changed.` };
+  if (owned.length > 0) {
+    const shown = owned.slice(0, 10).join(', ');
+    return {
+      pass: false,
+      reason: 'protected',
+      message: `This changes ${owned.length} protected file(s) (${shown}${owned.length > 10 ? ', ...' : ''}), so it needs an approval from another Team Mobile member.`,
+    };
   }
-  if (await trusted(get, author)) {
-    return { pass: true, message: `${author} has maintain or admin access, so their change to protected files needs no other approval.` };
-  }
-  for (const login of approvers(await paged(get, `/pulls/${number}/reviews`), author)) {
-    if (await trusted(get, login)) {
-      return { pass: true, message: `${login} approved, and has maintain or admin access.` };
-    }
+  const coderabbit = reviews.findLast(review => review.user?.login === CODERABBIT && review.state !== 'COMMENTED' && review.state !== 'PENDING');
+  if (coderabbit?.state !== 'APPROVED' || coderabbit.commit_id !== pull.head.sha) {
+    return { pass: false, reason: 'coderabbit', message: "CodeRabbit hasn't approved this commit. If it rates the merge risk high or critical, it won't, and this needs an approval from another Team Mobile member." };
   }
-  const shown = owned.slice(0, 10).join(', ');
-  return {
-    pass: false,
-    message: `This changes ${owned.length} protected file(s) (${shown}${owned.length > 10 ? ', ...' : ''}). It needs an approval from someone with maintain or admin access.`,
-  };
+  return { pass: true, message: `${author} has maintain or admin access, and CodeRabbit approved.` };
 }
 
 async function main(env, { fetch = globalThis.fetch, out = process.stdout } = {}) {

.github/scripts/code-owner-approval.test.mjs

@@ -25,7 +25,7 @@ function fakeGitHub({ list = PROTECTED_PATHS, files, changed = files.length, rev
       return list === null ? notFound : reply({ content: Buffer.from(list).toString('base64') });
     }
     if (path === '/pulls/7') {
-      return reply({ changed_files: changed });
+      return reply({ changed_files: changed, head: { sha: HEAD } });
     }
     const page = Number(searchParams.get('page'));
     const slice = items => items.slice((page - 1) * 100, page * 100);
@@ -52,7 +52,8 @@ async function run(github, author = 'outside-dev') {
   return { pass, output: lines.join('') };
 }
 
-const review = (login, state) => ({ user: { login }, state });
+const HEAD = 'c14cacc2763d2dfc271d9c286b09376a90c5b9ff';
+const review = (login, state, commit = HEAD) => ({ user: { login }, state, commit_id: commit });
 
 describe('protectedFiles', () => {
   const patterns = parseProtectedPaths(PROTECTED_PATHS);
@@ -70,40 +71,63 @@ describe('protectedFiles', () => {
 });
 
 describe('code owner approval', () => {
-  it('passes when no protected file changed', async () => {
-    const { pass, output } = await run({ files: [{ filename: 'Sources/ClerkKit/Clerk.swift' }] });
+  const coderabbit = review('coderabbitai[bot]', 'APPROVED');
+  const ordinary = [{ filename: 'Sources/ClerkKit/Clerk.swift' }];
+  const workflow = [{ filename: '.github/workflows/pr-ci.yml' }];
+
+  it('passes a Team Mobile author on ordinary files once CodeRabbit approves', async () => {
+    const { pass, output } = await run({ files: ordinary, reviews: [coderabbit], roles: { mikepitre: 'admin' } }, 'mikepitre');
     assert.equal(pass, true);
-    assert.match(output, /No file listed in \.github\/protected-paths changed/);
+    assert.match(output, /mikepitre has maintain or admin access, and CodeRabbit approved/);
+  });
+
+  it('fails a Team Mobile author until CodeRabbit approves, as when it rates the risk high', async () => {
+    const { pass, output } = await run({ files: ordinary, reviews: [review('coderabbitai[bot]', 'COMMENTED')], roles: { mikepitre: 'admin' } }, 'mikepitre');
+    assert.equal(pass, false);
+    assert.match(output, /CodeRabbit hasn't approved this commit/);
   });
 
-  it('passes when the author has maintain or admin access', async () => {
-    const { pass } = await run({ files: [{ filename: '.github/workflows/pr-ci.yml' }], roles: { mikepitre: 'admin' } }, 'mikepitre');
+  it('fails a Team Mobile author when CodeRabbit approved an earlier commit', async () => {
+    const stale = review('coderabbitai[bot]', 'APPROVED', 'e225ce56d448093f7910a36cf47bcc9a664e04d0');
+    const { pass } = await run({ files: ordinary, reviews: [stale], roles: { mikepitre: 'admin' } }, 'mikepitre');
+    assert.equal(pass, false);
+  });
+
+  it('fails a Team Mobile author on protected files even after CodeRabbit approves', async () => {
+    const { pass, output } = await run({ files: workflow, reviews: [coderabbit], roles: { swolfand: 'maintain' } }, 'swolfand');
+    assert.equal(pass, false);
+    assert.match(output, /This changes 1 protected file\(s\) \(\.github\/workflows\/pr-ci\.yml\), so it needs an approval from another Team Mobile member/);
+  });
+
+  it('passes a Team Mobile author on protected files once another member approves', async () => {
+    const { pass, output } = await run({ files: workflow, reviews: [review('seanperez29', 'APPROVED')], roles: { swolfand: 'maintain', seanperez29: 'admin' } }, 'swolfand');
     assert.equal(pass, true);
+    assert.match(output, /seanperez29 approved/);
   });
 
-  it('fails for an author with write access and no trusted approval', async () => {
+  it('fails an author with write access on ordinary files even after CodeRabbit and a writer approve', async () => {
     const { pass, output } = await run({
-      files: [{ filename: '.github/workflows/pr-ci.yml' }],
-      reviews: [review('coderabbitai[bot]', 'APPROVED'), review('writer', 'APPROVED')],
+      files: ordinary,
+      reviews: [coderabbit, review('writer', 'APPROVED')],
       roles: { 'outside-dev': 'write', writer: 'write' },
     });
     assert.equal(pass, false);
-    assert.match(output, /^::error title=Code owner approval::This changes 1 protected file\(s\) \(\.github\/workflows\/pr-ci\.yml\)/);
+    assert.match(output, /^::error title=Code owner approval::outside-dev doesn't have maintain or admin access/);
   });
 
-  it('passes on an approval from someone with maintain access', async () => {
-    const { pass, output } = await run({
-      files: [{ filename: 'Package.swift' }],
-      reviews: [review('swolfand', 'APPROVED')],
-      roles: { 'outside-dev': 'write', swolfand: 'maintain' },
-    });
+  it('fails a contributor with no access to the repository', async () => {
+    const { pass } = await run({ files: ordinary, reviews: [coderabbit] }, 'fork-contributor');
+    assert.equal(pass, false);
+  });
+
+  it('passes an author with write access once Team Mobile approves', async () => {
+    const { pass } = await run({ files: workflow, reviews: [review('wobsoriano', 'APPROVED')], roles: { 'outside-dev': 'write', wobsoriano: 'maintain' } });
     assert.equal(pass, true);
-    assert.match(output, /swolfand approved/);
   });
 
   it('ignores an approval that was later dismissed or followed by a change request', async () => {
     const { pass } = await run({
-      files: [{ filename: 'Package.swift' }],
+      files: ordinary,
       reviews: [review('swolfand', 'APPROVED'), review('swolfand', 'DISMISSED'), review('seanperez29', 'APPROVED'), review('seanperez29', 'COMMENTED'), review('seanperez29', 'CHANGES_REQUESTED')],
       roles: { swolfand: 'maintain', seanperez29: 'admin' },
     });
@@ -112,33 +136,33 @@ describe('code owner approval', () => {
 
   it('keeps an approval when a later review only comments', async () => {
     const { pass } = await run({
-      files: [{ filename: 'Package.swift' }],
+      files: ordinary,
       reviews: [review('seanperez29', 'APPROVED'), review('seanperez29', 'COMMENTED')],
       roles: { seanperez29: 'admin' },
     });
     assert.equal(pass, true);
   });
 
   it('counts the old path of a renamed file', async () => {
-    const { pass } = await run({ files: [{ filename: 'Old/Identity.swift', previous_filename: 'Sources/ClerkKit/Identity/Store.swift' }] });
+    const { pass } = await run({ files: [{ filename: 'Old/Identity.swift', previous_filename: 'Sources/ClerkKit/Identity/Store.swift' }], reviews: [coderabbit], roles: { mikepitre: 'admin' } }, 'mikepitre');
     assert.equal(pass, false);
   });
 
   it('reads every page of changed files', async () => {
     const files = Array.from({ length: 150 }, (_, i) => ({ filename: `Sources/File${i}.swift` }));
     files.push({ filename: '.github/workflows/late.yml' });
-    const { pass } = await run({ files });
+    const { pass } = await run({ files, reviews: [coderabbit], roles: { mikepitre: 'admin' } }, 'mikepitre');
     assert.equal(pass, false);
   });
 
   it('fails when GitHub lists fewer files than the pull request changed', async () => {
-    const { pass, output } = await run({ files: [{ filename: 'Sources/ClerkKit/Clerk.swift' }], changed: 3001 });
+    const { pass, output } = await run({ files: ordinary, changed: 3001, reviews: [coderabbit], roles: { mikepitre: 'admin' } }, 'mikepitre');
     assert.equal(pass, false);
     assert.match(output, /listed 1 of the 3001 changed files/);
   });
 
-  it('passes when the base branch has no protected-paths list', async () => {
-    const { pass } = await run({ list: null, files: [{ filename: '.github/workflows/pr-ci.yml' }] });
+  it('treats every file as unprotected when the base branch has no protected-paths list', async () => {
+    const { pass } = await run({ list: null, files: workflow, reviews: [coderabbit], roles: { mikepitre: 'admin' } }, 'mikepitre');
     assert.equal(pass, true);
   });
 });

.github/scripts/team-review-request.mjs

@@ -0,0 +1,83 @@
+import fs from 'node:fs';
+import { fileURLToPath } from 'node:url';
+
+import { codeOwnerApproval } from './code-owner-approval.mjs';
+import { client, paged } from './github.mjs';
+
+const TEAM_FILE = new URL('../team-mobile', import.meta.url);
+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 => {
+    if (comment.user?.login !== CODERABBIT) {
+      return false;
+    }
+    const body = comment.body ?? '';
+    const covered = /<!-- final_review_risk_coverage:(\{.*?\}) -->/.exec(body)?.[1];
+    const level = /<!-- final_review_risk_label\s*\*\*Merge Risk:\*\*\s*\*\*[^A-Za-z]*([A-Za-z]+)\*\*/.exec(body)?.[1];
+    return covered !== undefined && level !== undefined && JSON.parse(covered).coveredCommitId === head && HIGH_RISK.has(level.toLowerCase());
+  });
+}
+
+async function teamReviewRequest({ fetch, api, token, repo, number, team }) {
+  const get = client({ fetch, api, token, repo });
+  const pull = await get(`/pulls/${number}`);
+  if (pull.state !== 'open' || pull.draft) {
+    return 'The pull request is closed or a draft.';
+  }
+  const author = pull.user.login;
+  const check = await codeOwnerApproval({ fetch, api, token, repo, number, author, baseRef: pull.base.ref });
+  if (check.pass) {
+    return `No review request needed. ${check.message}`;
+  }
+  if (check.reason === 'coderabbit' && !ratedHighRisk(await paged(get, `/issues/${number}/comments`), pull.head.sha)) {
+    return 'Waiting on CodeRabbit, which hasn\'t rated this commit high or critical risk.';
+  }
+  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 reviewers = team.filter(login => !asked.has(login));
+  if (reviewers.length === 0) {
+    return 'Every Team Mobile member is already requested or has reviewed.';
+  }
+  await get(`/pulls/${number}/requested_reviewers`, { method: 'POST', body: { reviewers } });
+  const why = check.reason === 'coderabbit' ? 'CodeRabbit rated this commit high or critical risk.' : check.message;
+  return `Requested ${reviewers.join(', ')}. ${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]);
+  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,
+    repo: env.GITHUB_REPOSITORY,
+    number: env.PR_NUMBER,
+    team: parseTeam(teamText),
+  });
+  out.write(`${message}\n`);
+}
+
+export { main, parseTeam, ratedHighRisk };
+
+if (process.argv[1] !== undefined && fs.realpathSync(process.argv[1]) === fileURLToPath(import.meta.url)) {
+  try {
+    await main(process.env);
+  } catch (error) {
+    process.stderr.write(`${error.message}\n`);
+    process.exitCode = 1;
+  }
+}