ci: stop requesting reviewers when a Team Mobile author changes a protected path

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

@@ -28,16 +28,15 @@ function ratedHighRisk(comments, head) {
   });
 }
 
+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.";
+
 function protectedComment(files) {
   const shown = files.slice(0, 20).map(file => `- \`${file}\``);
   if (files.length > 20) {
     shown.push(`- and ${files.length - 20} more`);
   }
-  return [
-    "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.",
-    '',
-    ...shown,
-  ].join('\n');
+  return [PROTECTED_NOTICE, '', ...shown].join('\n');
 }
 
 async function teamReviewRequest({ fetch, api, token, repo, number, team }) {
@@ -54,6 +53,19 @@ async function teamReviewRequest({ fetch, api, token, repo, number, team }) {
   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.';
   }
+  if (check.reason === 'protected') {
+    const comments = await paged(get, `/issues/${number}/comments`);
+    const body = protectedComment(check.files);
+    const notice = comments.find(comment => comment.body?.startsWith(PROTECTED_NOTICE));
+    if (notice === undefined) {
+      await get(`/issues/${number}/comments`, { method: 'POST', body: { body } });
+    } else if (notice.body !== body) {
+      await get(`/issues/comments/${notice.id}`, { method: 'PATCH', body: { body } });
+    }
+    if (!ratedHighRisk(comments, pull.head.sha)) {
+      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);
@@ -63,10 +75,7 @@ async function teamReviewRequest({ fetch, api, token, repo, number, team }) {
     return 'Every Team Mobile member is already requested or has reviewed.';
   }
   await get(`/pulls/${number}/requested_reviewers`, { method: 'POST', body: { reviewers } });
-  if (check.reason === 'protected') {
-    await get(`/issues/${number}/comments`, { method: 'POST', body: { body: protectedComment(check.files) } });
-  }
-  const why = check.reason === 'coderabbit' ? 'CodeRabbit rated this commit high or critical risk.' : check.message;
+  const why = check.reason === 'coderabbit' || check.reason === 'protected' ? 'CodeRabbit rated this commit high or critical risk.' : check.message;
   return `Requested ${reviewers.join(', ')}. ${why}`;
 }
 

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

@@ -20,6 +20,7 @@ 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 = [] }) {
   const requested = [];
   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 }) => {
@@ -33,6 +34,11 @@ function fakeGitHub({ author = 'outside-dev', draft = false, state = 'open', fil
       posted.push(JSON.parse(body).body);
       return reply({});
     }
+    const comment = /^\/issues\/comments\/(\d+)$/.exec(path);
+    if (method === 'PATCH' && comment) {
+      edited.push({ id: Number(comment[1]), body: JSON.parse(body).body });
+      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 })) });
     }
@@ -50,17 +56,17 @@ function fakeGitHub({ author = 'outside-dev', draft = false, state = 'open', fil
     }
     throw new Error(`unexpected ${method} ${path}`);
   };
-  return { fetch, requested, posted };
+  return { fetch, requested, posted, edited };
 }
 
 async function run(github) {
-  const { fetch, requested, posted } = fakeGitHub(github);
+  const { fetch, requested, 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 },
   );
-  return { requested, posted, output: lines.join('') };
+  return { requested, posted, edited, output: lines.join('') };
 }
 
 describe('ratedHighRisk', () => {
@@ -96,24 +102,41 @@ describe('team review request', () => {
     assert.match(output, /CodeRabbit rated this commit high or critical risk/);
   });
 
-  it('requests the other members and explains why when a Team Mobile author changes a protected file', async () => {
+  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 });
-    assert.deepEqual(requested, ['seanperez29', 'swolfand', 'wobsoriano']);
+    assert.deepEqual(requested, []);
     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);
     assert.doesNotMatch(posted[0], /Clerk\.swift/);
   });
 
-  it('comments only when it requests someone new', async () => {
-    const { requested, posted } = await run({
-      author: 'mikepitre',
-      files: [{ filename: '.github/workflows/pr-ci.yml' }],
-      pending: ['seanperez29', 'swolfand', 'wobsoriano'],
-    });
-    assert.deepEqual(requested, []);
-    assert.deepEqual(posted, []);
+  it('leaves the protected-file notice alone when the protected files are the same', async () => {
+    const files = [{ filename: '.github/workflows/pr-ci.yml' }];
+    const first = await run({ author: 'mikepitre', files });
+    const again = await run({ author: 'mikepitre', files, comments: [{ id: 11, user: { login: 'github-actions[bot]' }, body: first.posted[0] }] });
+    assert.deepEqual(again.requested, []);
+    assert.deepEqual(again.posted, []);
+    assert.deepEqual(again.edited, []);
+  });
+
+  it('updates the protected-file notice when a later push changes the protected files', async () => {
+    const first = await run({ author: 'mikepitre', files: [{ filename: '.github/workflows/pr-ci.yml' }] });
+    const files = [{ filename: '.github/workflows/release.yml' }];
+    const again = await run({ author: 'mikepitre', files, comments: [{ id: 11, user: { login: 'github-actions[bot]' }, body: first.posted[0] }] });
+    assert.deepEqual(again.posted, []);
+    assert.equal(again.edited.length, 1);
+    assert.equal(again.edited[0].id, 11);
+    assert.match(again.edited[0].body, /^- `\.github\/workflows\/release\.yml`$/m);
+    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']);
+    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 () => {

CONTRIBUTING.md

@@ -140,11 +140,12 @@ unless the PR changes a path in `.github/protected-paths` or CodeRabbit rates it
 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.
-When it requests reviewers because a Team Mobile author changed a protected path, it also
-comments on the PR with the protected files. 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.
+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
+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.
 Authors can't use `@coderabbitai approve` on their own PRs or override a failing check.
 
 PRs merge through a merge queue. PR CI and each queue run do the lint, the unit tests,