Avoid repeated pending Review writes and draft Test polling

.github/workflows/ready.yml

@@ -80,9 +80,27 @@ jobs:
               if (pull.state !== 'open' || pull.base.ref !== repository.default_branch) return;
               const sha = pull.head.sha;
               const requiredTests = JSON.parse(process.env.TEST_CHECKS || '[]');
-              if (requiredTests.length) await github.rest.repos.createCommitStatus({owner, repo, sha,
-                context: 'Review', state: 'pending', description: 'Checking the current Test run and attempt.',
-                target_url: pull.html_url});
+              const reviewTarget = {owner, repo, sha, context: 'Review', target_url: pull.html_url};
+              let review;
+              async function setReview(state, description) {
+                // Pending already blocks publication, regardless of its wording.
+                if (review?.state === state && (state === 'pending' || review.description === description)) return;
+                await github.rest.repos.createCommitStatus({...reviewTarget, state, description});
+                review = {state, description};
+              }
+              try {
+                const result = await github.rest.repos.getCombinedStatusForRef({owner, repo, ref: sha, per_page: 100});
+                review = result.data.statuses.find(status => status.context === 'Review');
+              } catch (error) {
+                // A failed status read must not leave a previous success in force.
+                await setReview('pending', 'Unable to verify the current Review status.');
+                throw error;
+              }
+              if (pull.draft) {
+                await setReview('pending', 'Draft: publication is not authorized.');
+                return;
+              }
+              if (requiredTests.length) await setReview('pending', 'Checking the current Test run and attempt.');
               async function testsPassed() {
                 if (!requiredTests.length) return true;
                 // Read the current run and attempt on every event. Check runs can
@@ -126,12 +144,9 @@ jobs:
               if (current.state !== 'open' || current.base.ref !== pull.base.ref
                 || current.head.sha !== sha || current.draft !== pull.draft) return;
               const reviewPassed = !pull.draft && approved && await testsPassed();
-              await github.rest.repos.createCommitStatus({owner, repo, sha, context: 'Review',
-                state: reviewPassed ? 'success' : 'pending',
-                description: pull.draft ? 'Draft: publication is not authorized.'
-                  : reviewPassed ? 'Ready requirements passed for this revision.'
-                  : 'Waiting for required tests of this revision.',
-                target_url: pull.html_url});
+              await setReview(reviewPassed ? 'success' : 'pending', reviewPassed
+                ? 'Ready requirements passed for this revision.'
+                : 'Waiting for required tests of this revision.');
               if (!reviewPassed) return;
               // Agents own code merges so they can follow the production deployment.
               // Keep the customer's CloudCannon Publish button automatic.
@@ -163,8 +178,7 @@ jobs:
               if (final.state !== 'open' || final.draft || final.base.ref !== pull.base.ref || final.head.sha !== sha) return;
               if (!isCloudCannonPublication(final)) return;
               if (!await testsPassed()) {
-                await github.rest.repos.createCommitStatus({owner, repo, sha, context: 'Review',
-                  state: 'pending', description: 'Waiting for required tests of this revision.', target_url: pull.html_url});
+                await setReview('pending', 'Waiting for required tests of this revision.');
                 return;
               }
               const method = final.head.ref === 'preview' && final.head.repo?.full_name === final.base.repo?.full_name

tests/ready.test.mjs

@@ -28,6 +28,10 @@ async function scenario(options = {}) {
       pulls: {get: async ({pull_number}) => ({data: ++reads >= 2 && options.changed ? {...pull, head: {sha: 'new'}} : pulls.find(candidate => candidate.number === pull_number)}),
         list: 'pulls', merge: async args => { if (options.rejectPull === args.pull_number) throw Object.assign(new Error('Head changed'), {status: 409}); merges.push(args); }},
       repos: {get: async () => ({data: {default_branch: 'main', full_name: 'o/r', allow_auto_merge: options.autoMerge !== false}}),
+        getCombinedStatusForRef: async () => {
+          if (options.statusError) throw new Error('Status unavailable');
+          return {data: {statuses: options.review ? [{context: 'Review', ...options.review}] : []}};
+        },
         createCommitStatus: async status => statuses.push(status)},
       checks: {listForRef: 'checks'},
       actions: {listWorkflowRunsForRepo: 'runs', listJobsForWorkflowRunAttempt: 'jobs'},
@@ -53,7 +57,7 @@ async function scenario(options = {}) {
       TEST_CHECKS: JSON.stringify(options.testChecks ?? []),
       GITHUB_API_URL: 'https://api.github.com', MERGE_TOKEN: options.mergeToken}},
     options.fetch ?? (() => { throw new Error('Unexpected network request'); }), resolve => resolve());
-  return {statuses, merges, errors};
+  return {statuses, merges, errors, runReads};
 }
 
 test('CloudCannon publication with real checks enables auto-merge', async () => {
@@ -66,6 +70,37 @@ test('draft remains pending and cannot auto-merge', async () => {
   assert.equal(result.statuses[0].state, 'pending');
   assert.equal(result.merges.length, 0);
 });
+test('pending drafts do not append statuses or query Test metadata on repeated events', async () => {
+  for (const description of ['Draft: publication is not authorized.', 'Checking the current Test run and attempt.']) {
+    const result = await scenario({pull: {draft: true}, testChecks: ['Test / Test'],
+      review: {state: 'pending', description}, runRead: () => {throw new Error('Draft must not query Test');}});
+    assert.deepEqual(result.statuses, []);
+    assert.equal(result.runReads, 0);
+    assert.deepEqual(result.errors, []);
+    assert.deepEqual(result.merges, []);
+  }
+});
+test('converting a successful PR to draft revokes its old Review', async () => {
+  const result = await scenario({pull: {draft: true}, testChecks: ['Test / Test'],
+    review: {state: 'success'}, runRead: () => {throw new Error('Draft must not query Test');}});
+  assert.equal(result.statuses.length, 1);
+  assert.equal(result.statuses[0].state, 'pending');
+  assert.deepEqual(result.errors, []);
+  assert.deepEqual(result.merges, []);
+});
+test('an already-pending ready PR waits without another status write', async () => {
+  const result = await scenario({testChecks: ['Test / Test'], review: {state: 'pending'},
+    runs: [{id: 100, name: 'Test', path: '.github/workflows/test.yml', head_sha: 'head', status: 'in_progress'}]});
+  assert.deepEqual(result.statuses, []);
+  assert.deepEqual(result.errors, []);
+  assert.deepEqual(result.merges, []);
+});
+test('a status read failure revokes any previous success before failing Ready', async () => {
+  const result = await scenario({statusError: true, review: {state: 'success'}});
+  assert.equal(result.statuses.at(-1).state, 'pending');
+  assert.match(result.errors[0], /Status unavailable/);
+  assert.deepEqual(result.merges, []);
+});
 test('skipped, failed or absent static checks never authorize publication', async () => {
   for (const checks of [[], [{id: 1, name: 'Check / Check', app: {id: 15368}, conclusion: 'skipped'}],
     [{id: 1, name: 'Check / Check', app: {id: 15368}, conclusion: 'failure'}]]) {
@@ -282,7 +317,8 @@ test('persistent missing metadata is bounded and API errors revoke old success',
   const missing = await scenario({...trial, runRead: () => {reads++; return [];}});
   assert.equal(reads, 4);
   assert.equal(missing.statuses.at(-1).state, 'pending');
-  const failed = await scenario({...trial, runRead: () => {throw new Error('Metadata unavailable');}});
+  const failed = await scenario({...trial, review: {state: 'success'},
+    runRead: () => {throw new Error('Metadata unavailable');}});
   assert.equal(failed.statuses.at(-1).state, 'pending');
   assert.match(failed.errors[0], /Metadata unavailable/);
 });