From fd174e69c3b67b836d14c7993d0fe594a3309950 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 17:34:10 +0000 Subject: [PATCH] fix: evaluate backport approval from live labels to prevent check race On manually-opened backport PRs, the shared pull_request handler evaluated the Backport Approval Enforcement verdict from the webhook payload's label snapshot, which is frozen at delivery time and can never contain the backport/requested label that updateManualBackport adds mid-handler. A slow invocation carrying a stale payload could then complete the check run last with 'Backport Approval Not Required', and the labeled-event safety net no-op'd whenever the run was already queued, so the stale conclusion stood and auto-merge could proceed without approval. - Evaluate the approval verdict from live labels fetched via the API (labelExistsOnPR) instead of the payload's label array. - Replace the queued no-op with an idempotent write of the pending state, so a labeled/unlabeled event corrects a stale conclusion instead of returning early. - Deduplicate check-run creation: queueBackportApprovalCheck now re-lists check runs for the head SHA and resets the existing run to queued by id instead of creating another run. - When backport/approved is removed, keep the check pending after re-adding backport/requested rather than stamping success. --- spec/checks-util.spec.ts | 69 +++++++++++++++++++ spec/index.spec.ts | 143 +++++++++++++++++++++++++++++++++++++-- src/index.ts | 44 ++++++++---- src/utils/checks-util.ts | 31 +++++++-- 4 files changed, 264 insertions(+), 23 deletions(-) create mode 100644 spec/checks-util.spec.ts diff --git a/spec/checks-util.spec.ts b/spec/checks-util.spec.ts new file mode 100644 index 0000000..f92ae6c --- /dev/null +++ b/spec/checks-util.spec.ts @@ -0,0 +1,69 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { BACKPORT_APPROVAL_CHECK } from '../src/constants'; +import { queueBackportApprovalCheck } from '../src/utils/checks-util'; + +const backportPROpenedEvent = require('./fixtures/backport_pull_request.opened.json'); + +describe('checks-util', () => { + describe('queueBackportApprovalCheck', () => { + const octokit = { + checks: { + listForRef: vi.fn(), + create: vi.fn().mockResolvedValue({ data: {} }), + update: vi.fn().mockResolvedValue({ data: {} }), + }, + }; + + const context = { + ...backportPROpenedEvent, + octokit, + repo: vi.fn((obj) => obj), + }; + + beforeEach(() => vi.clearAllMocks()); + + it('creates a new queued check run when none exists', async () => { + octokit.checks.listForRef.mockResolvedValue({ + data: { check_runs: [] }, + }); + + await queueBackportApprovalCheck(context); + + expect(octokit.checks.create).toHaveBeenCalledTimes(1); + expect(octokit.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + name: BACKPORT_APPROVAL_CHECK, + status: 'queued', + }), + ); + expect(octokit.checks.update).not.toHaveBeenCalled(); + }); + + it('resets the existing check run to queued instead of creating a duplicate', async () => { + octokit.checks.listForRef.mockResolvedValue({ + data: { + check_runs: [ + { + id: 12345, + name: BACKPORT_APPROVAL_CHECK, + status: 'completed', + conclusion: 'success', + }, + ], + }, + }); + + 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', + }), + ); + }); + }); +}); diff --git a/spec/index.spec.ts b/spec/index.spec.ts index dc4420c..8886feb 100644 --- a/spec/index.spec.ts +++ b/spec/index.spec.ts @@ -554,12 +554,44 @@ describe('trop', () => { .get( `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`, ) - .reply(200, []); + .reply(200, [backportRequestedLabel]); await robot.receive(event); - expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(1); + // 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.updateBackportApprovalCheck).not.toHaveBeenCalled(); + }); + + it('does not conclude "Not Required" when "backport/requested" was added after the payload snapshot', async () => { + // Regression test for the manual-backport race: the `opened` handler + // itself adds `backport/requested` via updateManualBackport, so the + // payload's label snapshot can never contain it. The verdict must be + // evaluated from the live labels, not the stale snapshot. + 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); + + // Live labels include `backport/requested` even though the payload + // snapshot (fixture) does not. + nock(GH_API) + .persist() + .get( + '/repos/codebytere/probot-test/issues/7/labels?per_page=100&page=1', + ) + .reply(200, [backportRequestedLabel]); + + await robot.receive(backportPROpenedEvent); + + expect(updateManualBackport).toHaveBeenCalled(); expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled(); + expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(2); }); it('passes the backport approval check if the "backport/approved" label is on a new backport PR', async () => { @@ -583,7 +615,7 @@ describe('trop', () => { .get( `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`, ) - .reply(200, []); + .reply(200, [backportApprovedLabel]); await robot.receive(event); @@ -624,7 +656,7 @@ describe('trop', () => { await fs.readFile(backportPRLabeledEventPath, 'utf-8'), ); - event.payload.label = backportApprovedLabel; + event.payload.label = backportRequestedLabel; event.payload.pull_request.labels = [backportRequestedLabel]; nock(GH_API) @@ -654,7 +686,54 @@ describe('trop', () => { .get( `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`, ) - .reply(200, MOCK_PR.labels); + .reply(200, [backportRequestedLabel]); + + await robot.receive(event); + + expect(checkUtils.queueBackportApprovalCheck).toHaveBeenCalledTimes(1); + expect(checkUtils.updateBackportApprovalCheck).not.toHaveBeenCalled(); + }); + + it('re-asserts the pending state even when the check run is already queued', async () => { + // Regression test: this previously no-op'd when the run was already + // `queued`, letting a slower concurrent invocation holding a stale + // payload complete the run last with an incorrect conclusion. + 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: 'queued', + }, + ], + }); + + 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); @@ -662,6 +741,58 @@ describe('trop', () => { 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'), + ); + + event.payload.label = backportApprovedLabel; + event.payload.pull_request.labels = [backportApprovedLabel]; + + 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: 'queued', + }, + ], + }); + + 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, [backportApprovedLabel]); + + await robot.receive(event); + + expect(checkUtils.queueBackportApprovalCheck).not.toHaveBeenCalled(); + + const updatePayload = vi.mocked(checkUtils.updateBackportApprovalCheck) + .mock.calls[0][2]; + + expect(updatePayload).toMatchObject({ + title: 'Backport Approved', + summary: 'This PR has been approved for backporting.', + conclusion: CheckRunStatus.SUCCESS, + }); + }); + it('passes the backport approval check if the "backport/approved" label is added', async () => { const event = JSON.parse( await fs.readFile(backportPRLabeledEventPath, 'utf-8'), @@ -691,7 +822,7 @@ describe('trop', () => { .get( `/repos/codebytere/probot-test/issues/${event.payload.pull_request.number}/labels?per_page=100&page=1`, ) - .reply(200, MOCK_PR.labels); + .reply(200, [backportApprovedLabel]); await robot.receive(event); diff --git a/src/index.ts b/src/index.ts index d823ed6..5ca2acd 100644 --- a/src/index.ts +++ b/src/index.ts @@ -378,11 +378,21 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { } } - const isBackportApproved = pr.labels.some( - (prLabel) => prLabel.name === BACKPORT_APPROVED_LABEL, + // Evaluate the approval verdict from the live labels on the PR + // rather than the webhook payload's label snapshot. The payload is + // frozen at delivery time, so it can never contain a label added + // mid-handler (e.g. `backport/requested` added for a manual backport + // by updateManualBackport above) and a slow invocation racing with + // other deliveries would otherwise stamp a stale verdict last. + const isBackportApproved = await labelExistsOnPR( + context, + pr.number, + BACKPORT_APPROVED_LABEL, ); - const isBackportRequested = pr.labels.some( - (prLabel) => prLabel.name === BACKPORT_REQUESTED_LABEL, + const isBackportRequested = await labelExistsOnPR( + context, + pr.number, + BACKPORT_REQUESTED_LABEL, ); if (isBackportApproved) { @@ -396,22 +406,30 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => { conclusion: CheckRunStatus.SUCCESS, }); } else if (!isBackportRequested) { - // If backport/approved was removed, add backport/requested back if ( action === 'unlabeled' && label?.name === BACKPORT_APPROVED_LABEL ) { + // If backport/approved was removed, add backport/requested back + // and keep the check pending - the same invocation that flags a + // PR as needing approval must never conclude that approval is + // not required. await addLabels(context, pr.number, [BACKPORT_REQUESTED_LABEL]); - } - await updateBackportApprovalCheck(context, backportApprovalCheck, { - title: 'Backport Approval Not Required', - summary: 'This PR does not need backport approval.', - conclusion: CheckRunStatus.SUCCESS, - }); - } else { - if (backportApprovalCheck.status !== 'queued') { await queueBackportApprovalCheck(context); + } else { + await updateBackportApprovalCheck(context, backportApprovalCheck, { + title: 'Backport Approval Not Required', + summary: 'This PR does not need backport approval.', + conclusion: CheckRunStatus.SUCCESS, + }); } + } else { + // Always (re-)assert the pending state, even when the check run + // already appears queued in our snapshot - a concurrent invocation + // holding a stale payload may have completed it (or may be about + // to). An idempotent write of the queued state corrects a stale + // conclusion instead of silently trusting the snapshot. + await queueBackportApprovalCheck(context); } } else { // If we're somehow targeting main and have a check run, diff --git a/src/utils/checks-util.ts b/src/utils/checks-util.ts index 35910d1..0b5ae6c 100644 --- a/src/utils/checks-util.ts +++ b/src/utils/checks-util.ts @@ -141,16 +141,39 @@ export async function updateBackportApprovalCheck( export async function queueBackportApprovalCheck(context: WebHookPRContext) { const pr = context.payload.pull_request; + const output = { + title: 'Needs Backport Approval', + summary: 'This PR requires backport approval.', + }; + + // 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. + const existingCheck = await getBackportApprovalCheck(context); + + if (existingCheck) { + await context.octokit.checks.update( + context.repo({ + check_run_id: existingCheck.id, + name: existingCheck.name, + status: 'queued' as 'queued', + details_url: 'https://github.com/electron/trop', + output, + }), + ); + return; + } + await context.octokit.checks.create( context.repo({ name: BACKPORT_APPROVAL_CHECK, head_sha: pr.head.sha, status: 'queued', details_url: 'https://github.com/electron/trop', - output: { - title: 'Needs Backport Approval', - summary: 'This PR requires backport approval.', - }, + output, }), ); }