fix: supersede completed backport approval check runs instead of updating them

spec/checks-util.spec.ts

@@ -40,7 +40,42 @@ describe('checks-util', () => {
       expect(octokit.checks.update).not.toHaveBeenCalled();
     });
 
-    it('resets the existing check run to queued instead of creating a duplicate', async () => {
+    it.each(['queued', 'in_progress'])(
+      'resets an existing %s check run to queued instead of creating a duplicate',
+      async (status) => {
+        octokit.checks.listForRef.mockResolvedValue({
+          data: {
+            check_runs: [
+              {
+                id: 12345,
+                name: BACKPORT_APPROVAL_CHECK,
+                status,
+                conclusion: null,
+              },
+            ],
+          },
+        });
+
+        await queueBackportApprovalCheck(context);
+
+        expect(octokit.checks.create).not.toHaveBeenCalled();
+        expect(octokit.checks.update).toHaveBeenCalledTimes(1);
+        expect(octokit.checks.update).toHaveBeenCalledWith(
+          expect.objectContaining({
+            check_run_id: 12345,
+            status: 'queued',
+          }),
+        );
+      },
+    );
+
+    it('supersedes a completed check run with a fresh queued run instead of updating it', async () => {
+      // Regression test for electron/electron#53035: the Checks API treats
+      // a completed run as terminal, so a PATCH back to 'queued' silently
+      // keeps status=completed/conclusion=success and only rewrites the
+      // output - leaving a green check that reads "Needs Backport
+      // Approval". A fresh queued run must be created instead, which
+      // supersedes the completed one for branch protection.
       octokit.checks.listForRef.mockResolvedValue({
         data: {
           check_runs: [
@@ -56,11 +91,11 @@ describe('checks-util', () => {
 
       await queueBackportApprovalCheck(context);
 
-      expect(octokit.checks.create).not.toHaveBeenCalled();
-      expect(octokit.checks.update).toHaveBeenCalledTimes(1);
-      expect(octokit.checks.update).toHaveBeenCalledWith(
+      expect(octokit.checks.update).not.toHaveBeenCalled();
+      expect(octokit.checks.create).toHaveBeenCalledTimes(1);
+      expect(octokit.checks.create).toHaveBeenCalledWith(
         expect.objectContaining({
-          check_run_id: 12345,
+          name: BACKPORT_APPROVAL_CHECK,
           status: 'queued',
         }),
       );

spec/index.spec.ts

@@ -499,7 +499,20 @@ describe('trop', () => {
       expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
     });
 
-    it('passes the backport approval check if the "backport/requested" label is not on new backport PR', async () => {
+    it('keeps the backport approval check pending when a backport is opened before its labels have settled', async () => {
+      // Regression test for electron/electron#53035: a declared backport's
+      // labels are written by trop itself - backportImpl labels trop-created
+      // backports shortly after opening them, and updateManualBackport
+      // labels manually-opened backports. Regardless of author, the
+      // `opened` evaluation can therefore see no labels at all, and
+      // concluding "not required" in that window is premature - the verdict
+      // must stay pending until the labeled events that follow trop's label
+      // writes settle it.
+      getBackportApprovalCheck.mockResolvedValueOnce({
+        name: BACKPORT_APPROVAL_CHECK,
+        status: 'queued',
+      });
+
       nock(GH_API)
         .persist()
         .get('/repos/codebytere/probot-test/pulls/12345')
@@ -509,6 +522,32 @@ describe('trop', () => {
         .get('/repos/codebytere/probot-test/branches?protected=true')
         .reply(200, BRANCHES);
 
+      // The labels are still in flight - none have landed yet.
+      nock(GH_API)
+        .persist()
+        .get(
+          '/repos/codebytere/probot-test/issues/7/labels?per_page=100&page=1',
+        )
+        .reply(200, []);
+
+      await robot.receive(backportPROpenedEvent);
+
+      expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
+      // Once to create the missing check run, once to (re-)assert the
+      // pending state after evaluating the live labels.
+      expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(2);
+    });
+
+    it('concludes "Not Required" on opened for a release-branch PR with no backport declaration', async () => {
+      // PRs without a "Backport of #N" declaration (e.g. fast-track PRs)
+      // are never labeled by trop, so no labeled event is guaranteed to
+      // arrive. The `opened` evaluation must conclude for them - leaving
+      // the check queued would hang it forever.
+      // Consumed by the manual-backport scan and the declaration guard.
+      vi.mocked(getPRNumbersFromPRBody)
+        .mockReturnValueOnce([])
+        .mockReturnValueOnce([]);
+
       nock(GH_API)
         .persist()
         .get(
@@ -518,6 +557,8 @@ describe('trop', () => {
 
       await robot.receive(backportPROpenedEvent);
 
+      expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(1);
+
       const updatePayload = vi.mocked(checkUtils.updateBackportApprovalCheck)
         .mock.calls[0][2];
 
@@ -594,6 +635,56 @@ describe('trop', () => {
       expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(2);
     });
 
+    it('does not re-queue a concluded backport approval check when `opened` is delivered late', async () => {
+      // Webhook deliveries can be reordered: a labeled delivery processed
+      // before the `opened` one has already settled the verdict from the
+      // live labels. The late `opened` evaluation must not supersede that
+      // concluded run with a queued one - no follow-up event would ever
+      // complete it.
+
+      // Replace the beforeEach interceptor that reports no check runs.
+      nock.cleanAll();
+      nock(GH_API).post('/repos/codebytere/probot-test/check-runs').reply(200);
+
+      nock(GH_API)
+        .persist()
+        .get(
+          '/repos/codebytere/probot-test/commits/ABC/check-runs?per_page=100',
+        )
+        .reply(200, {
+          check_runs: [
+            {
+              name: BACKPORT_APPROVAL_CHECK,
+              status: 'completed',
+              conclusion: 'success',
+            },
+          ],
+        });
+
+      nock(GH_API)
+        .persist()
+        .get('/repos/codebytere/probot-test/pulls/12345')
+        .reply(200, MOCK_PR);
+
+      nock(GH_API)
+        .get('/repos/codebytere/probot-test/branches?protected=true')
+        .reply(200, BRANCHES);
+
+      // The backport labels trop added have landed and were already
+      // evaluated by the labeled deliveries that concluded the check.
+      nock(GH_API)
+        .persist()
+        .get(
+          '/repos/codebytere/probot-test/issues/7/labels?per_page=100&page=1',
+        )
+        .reply(200, [{ name: 'backport', color: 'fff' }]);
+
+      await robot.receive(backportPROpenedEvent);
+
+      expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
+      expect(checkUtils.queueBackportApprovalCheck).not.toHaveBeenCalled();
+    });
+
     it('passes the backport approval check if the "backport/approved" label is on a new backport PR', async () => {
       const event = JSON.parse(
         await fs.readFile(newPRBackportOpenedEventPath, 'utf-8'),
@@ -741,6 +832,56 @@ describe('trop', () => {
       expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
     });
 
+    it('re-asserts the pending state when a stale run already concluded success', async () => {
+      // Mirrors electron/electron#53035: the `opened` evaluation of a
+      // trop-created backport concluded success before trop added its
+      // labels. The subsequent labeled event must still (re-)assert the
+      // pending state so queueBackportApprovalCheck can supersede the
+      // completed run with a fresh queued one.
+      const event = JSON.parse(
+        await fs.readFile(backportPRLabeledEventPath, 'utf-8'),
+      );
+
+      event.payload.label = backportRequestedLabel;
+      event.payload.pull_request.labels = [backportRequestedLabel];
+
+      nock(GH_API)
+        .persist()
+        .get(
+          '/repos/codebytere/probot-test/commits/ABC/check-runs?per_page=100',
+        )
+        .reply(200, {
+          check_runs: [
+            {
+              name: BACKPORT_APPROVAL_CHECK,
+              status: 'completed',
+              conclusion: 'success',
+            },
+          ],
+        });
+
+      nock(GH_API)
+        .persist()
+        .get('/repos/codebytere/probot-test/pulls/12345')
+        .reply(200, MOCK_PR);
+
+      nock(GH_API)
+        .get('/repos/codebytere/probot-test/branches?protected=true')
+        .reply(200, BRANCHES);
+
+      nock(GH_API)
+        .persist()
+        .get(
+          `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`,
+        )
+        .reply(200, [backportRequestedLabel]);
+
+      await robot.receive(event);
+
+      expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(1);
+      expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled();
+    });
+
     it('completes a queued check when the "backport/approved" label is added', async () => {
       const event = JSON.parse(
         await fs.readFile(backportPRLabeledEventPath, 'utf-8'),

src/index.ts

@@ -416,6 +416,30 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => {
             // not required.
             await addLabels(context, pr.number, [BACKPORT_REQUESTED_LABEL]);
             await queueBackportApprovalCheck(context);
+          } else if (
+            action === 'opened' &&
+            getPRNumbersFromPRBody(pr).length > 0
+          ) {
+            // A declared backport's labels are written by trop, not by its
+            // author: backportImpl labels trop-created backports in a
+            // separate API call shortly after opening them, and
+            // updateManualBackport labels manually-opened backports (with
+            // at least the base-ref label). On `opened` those labels may
+            // still be in flight, so an empty label set must not conclude
+            // "not required" - keep the check pending and let the labeled
+            // events that follow trop's label writes settle the verdict.
+            // This applies to every declared backport regardless of author;
+            // PRs without a backport declaration (e.g. fast-track PRs) get
+            // no guaranteed labeled event, so they still conclude below.
+            //
+            // Webhook deliveries can be reordered: when a labeled delivery
+            // was processed before this `opened` one, the verdict was
+            // already settled from the same live labels consulted above -
+            // don't supersede a concluded run with a queued one that no
+            // follow-up event would ever complete.
+            if (backportApprovalCheck.status !== 'completed') {
+              await queueBackportApprovalCheck(context);
+            }
           } else {
             await updateBackportApprovalCheck(context, backportApprovalCheck, {
               title: 'Backport Approval Not Required',

src/utils/checks-util.ts

@@ -149,12 +149,11 @@ export async function queueBackportApprovalCheck(context: WebHookPRContext) {
   // Re-fetch the existing check run immediately before writing - concurrent
   // webhook deliveries race through check-then-create and would otherwise
   // create duplicate check runs for the same head SHA (branch protection
-  // only consults the latest run per name). If a run already exists - even
-  // one a stale invocation already completed - reset it to queued by id
-  // instead of creating another.
+  // only consults the latest run per name). If a run already exists and is
+  // still pending, reset it to queued by id instead of creating another.
   const existingCheck = await getBackportApprovalCheck(context);
 
-  if (existingCheck) {
+  if (existingCheck && existingCheck.status !== 'completed') {
     await context.octokit.checks.update(
       context.repo({
         check_run_id: existingCheck.id,
@@ -167,6 +166,12 @@ export async function queueBackportApprovalCheck(context: WebHookPRContext) {
     return;
   }
 
+  // A completed check run is terminal in the Checks API: a PATCH asking to
+  // move it back to 'queued' succeeds and applies the output, but silently
+  // keeps the old status and conclusion. Updating a stale completed run
+  // would therefore leave a green check that claims to need approval, so
+  // create a fresh queued run instead - branch protection only consults
+  // the latest run per name, so the new run supersedes the completed one.
   await context.octokit.checks.create(
     context.repo({
       name: BACKPORT_APPROVAL_CHECK,