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..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,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'), diff --git a/src/index.ts b/src/index.ts index 5ca2acd..dc6abe8 100644 --- a/src/index.ts +++ b/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', 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,