From 9b16aa0c56ea271157d15e944f5cbd49c7c8b22b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 21:26:23 +0000 Subject: [PATCH 1/2] fix: supersede completed backport approval check runs instead of updating them The Checks API treats a completed check run as terminal: a PATCH asking to move it back to 'queued' succeeds and applies the output, but silently keeps the old status and conclusion. The dedupe path added in #422 therefore could not un-green a run that had already concluded success - the labeled-event reset only rewrote the output text, leaving a green check whose output read 'Needs Backport Approval' (observed on electron/electron#53035). - queueBackportApprovalCheck now only updates an existing run in place while it is still pending; a completed run is superseded by a fresh queued run, which branch protection consults as the latest run per name. - The opened-event evaluation no longer concludes 'not required' for backport PRs authored by trop itself: trop adds labels in a separate API call shortly after creating the PR, so the live labels are still empty in that window. The check stays queued until the labeled events that follow trop's own label writes settle the verdict. --- spec/checks-util.spec.ts | 45 ++++++++++++++++++--- spec/index.spec.ts | 85 ++++++++++++++++++++++++++++++++++++++++ src/index.ts | 11 ++++++ src/utils/checks-util.ts | 13 ++++-- 4 files changed, 145 insertions(+), 9 deletions(-) diff --git a/spec/checks-util.spec.ts b/spec/checks-util.spec.ts index f92ae6c..d9dedb6 100644 --- a/spec/checks-util.spec.ts +++ b/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', }), ); diff --git a/spec/index.spec.ts b/spec/index.spec.ts index 8886feb..2a49e53 100644 --- a/spec/index.spec.ts +++ b/spec/index.spec.ts @@ -594,6 +594,41 @@ describe('trop', () => { expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(2); }); + it('keeps the backport approval check pending when trop opens its own backport before labeling it', async () => { + // Regression test for electron/electron#53035: trop labels its own + // backport PRs in a separate API call shortly after creating them, + // so the `opened` evaluation of a trop-created backport sees no + // labels at all. Concluding "not required" in that window is + // premature - the verdict must stay pending until the labeled + // events that follow trop's own label writes settle it. + const event = JSON.parse(JSON.stringify(backportPROpenedEvent)); + event.payload.pull_request.user.login = BOT_USER_NAME; + + 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); + + // trop has not added any labels to the freshly-created PR yet. + nock(GH_API) + .persist() + .get( + '/repos/codebytere/probot-test/issues/7/labels?per_page=100&page=1', + ) + .reply(200, []); + + await robot.receive(event); + + 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('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 +776,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'), diff --git a/src/index.ts b/src/index.ts index 5ca2acd..5925960 100644 --- a/src/index.ts +++ b/src/index.ts @@ -416,6 +416,17 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { // not required. await addLabels(context, pr.number, [BACKPORT_REQUESTED_LABEL]); await queueBackportApprovalCheck(context); + } else if ( + action === 'opened' && + pr.user.login === getEnvVar('BOT_USER_NAME') + ) { + // trop labels its own backport PRs in a separate API call + // shortly after creating them, so when the `opened` event for a + // trop-created backport is evaluated the live labels are still + // empty and a "not required" verdict would be premature. Keep + // the check pending instead - the labeled events that follow + // trop's own label writes re-evaluate and settle the verdict. + await queueBackportApprovalCheck(context); } else { await updateBackportApprovalCheck(context, backportApprovalCheck, { title: 'Backport Approval Not Required', diff --git a/src/utils/checks-util.ts b/src/utils/checks-util.ts index 0b5ae6c..2352116 100644 --- a/src/utils/checks-util.ts +++ b/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, From 09b7c5a08c4b40c5a8cf687909846f2d6567b098 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 22:50:58 +0000 Subject: [PATCH 2/2] fix: keep opened backport approval check pending for all declared backports The opened-event guard special-cased backports authored by trop's bot user, but the window it protects against is not author-specific: a declared backport's labels are always written by trop itself - backportImpl labels trop-created backports in a separate API call after opening them, and updateManualBackport labels manually-opened backports with at least the base-ref label - so labels can land after the opened delivery for any author, and webhook deliveries can be delayed or reordered besides. Gate the guard on the author-agnostic property that actually matters: whether the PR body declares 'Backport of #N'. Every declared backport is guaranteed at least one trop-written label and therefore a labeled event that settles the verdict, so opened can safely leave the check pending. PRs without a declaration (e.g. fast-track PRs targeting release branches) get no guaranteed labeled event and would hang queued forever, so opened still concludes 'not required' for them. Also skip the re-queue when the check has already concluded: a labeled delivery processed before a late opened delivery has already settled the verdict from the same live labels, and superseding that concluded run would leave a queued check no follow-up event ever completes. --- spec/index.spec.ts | 88 +++++++++++++++++++++++++++++++++++++--------- src/index.ts | 29 ++++++++++----- 2 files changed, 93 insertions(+), 24 deletions(-) diff --git a/spec/index.spec.ts b/spec/index.spec.ts index 2a49e53..e9811a8 100644 --- a/spec/index.spec.ts +++ b/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,15 +635,31 @@ describe('trop', () => { expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(2); }); - it('keeps the backport approval check pending when trop opens its own backport before labeling it', async () => { - // Regression test for electron/electron#53035: trop labels its own - // backport PRs in a separate API call shortly after creating them, - // so the `opened` evaluation of a trop-created backport sees no - // labels at all. Concluding "not required" in that window is - // premature - the verdict must stay pending until the labeled - // events that follow trop's own label writes settle it. - const event = JSON.parse(JSON.stringify(backportPROpenedEvent)); - event.payload.pull_request.user.login = BOT_USER_NAME; + 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() @@ -613,20 +670,19 @@ describe('trop', () => { .get('/repos/codebytere/probot-test/branches?protected=true') .reply(200, BRANCHES); - // trop has not added any labels to the freshly-created PR yet. + // 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, []); + .reply(200, [{ name: 'backport', color: 'fff' }]); - await robot.receive(event); + 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); + expect(checkUtils.queueBackportApprovalCheck).not.toHaveBeenCalled(); }); it('passes the backport approval check if the "backport/approved" label is on a new backport PR', async () => { diff --git a/src/index.ts b/src/index.ts index 5925960..dc6abe8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -418,15 +418,28 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { await queueBackportApprovalCheck(context); } else if ( action === 'opened' && - pr.user.login === getEnvVar('BOT_USER_NAME') + getPRNumbersFromPRBody(pr).length > 0 ) { - // trop labels its own backport PRs in a separate API call - // shortly after creating them, so when the `opened` event for a - // trop-created backport is evaluated the live labels are still - // empty and a "not required" verdict would be premature. Keep - // the check pending instead - the labeled events that follow - // trop's own label writes re-evaluate and settle the verdict. - await queueBackportApprovalCheck(context); + // 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',