From f9cd789f7cccef58bd3f204e48e62e0cfbe6edee Mon Sep 17 00:00:00 2001 From: Jesse Hills <3060199+jesserockz@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:39:36 +1200 Subject: [PATCH] [ci] Tell stacked pull requests apart from hand-built chains (#17980) --- .github/scripts/auto-label-pr/constants.js | 1 + .github/scripts/auto-label-pr/detectors.js | 39 ++++++- .github/scripts/auto-label-pr/index.js | 4 +- .../auto-label-pr/tests/detectors.test.js | 102 ++++++++++++++++++ .github/workflows/auto-label-pr.yml | 2 +- 5 files changed, 144 insertions(+), 4 deletions(-) diff --git a/.github/scripts/auto-label-pr/constants.js b/.github/scripts/auto-label-pr/constants.js index b95bc17518a..5e8acb09c96 100644 --- a/.github/scripts/auto-label-pr/constants.js +++ b/.github/scripts/auto-label-pr/constants.js @@ -13,6 +13,7 @@ module.exports = { 'merging-to-release', 'merging-to-beta', 'chained-pr', + 'stacked-pr', 'core', 'small-pr', 'medium-pr', diff --git a/.github/scripts/auto-label-pr/detectors.js b/.github/scripts/auto-label-pr/detectors.js index 2478ccf959d..bb85ccd681d 100644 --- a/.github/scripts/auto-label-pr/detectors.js +++ b/.github/scripts/auto-label-pr/detectors.js @@ -33,8 +33,41 @@ async function fetchPrFileContent(github, context, path) { } } +// Check whether a pull request is part of a GitHub stack. +// +// GitHub's stacked pull request feature adds a `stack` object to the pull +// request resource. It is present on every pull request in the stack - +// including the bottom one, whose base is already `dev` - and is absent +// entirely on standalone pull requests. +// +// The `pull_request_target` webhook payload is not guaranteed to carry this +// field, so fall back to asking the API when it is missing. Guessing wrong +// here is costly: a stacked pull request mistaken for a manually chained one +// gets a label that blocks merging. +async function isStackedPr(github, context) { + const pr = context.payload.pull_request; + if (pr.stack != null) { + return true; + } + + try { + const { owner, repo } = context.repo; + const { data } = await github.rest.pulls.get({ + owner, + repo, + pull_number: pr.number, + }); + return data.stack != null; + } catch (error) { + // Treat an API failure as "not stacked" so a chained pull request still + // gets its blocking label rather than silently slipping through. + console.log('Failed to check stack membership:', error.message); + return false; + } +} + // Strategy: Merge branch detection -async function detectMergeBranch(context) { +async function detectMergeBranch(github, context) { const labels = new Set(); const baseRef = context.payload.pull_request.base.ref; @@ -42,7 +75,11 @@ async function detectMergeBranch(context) { labels.add('merging-to-release'); } else if (baseRef === 'beta') { labels.add('merging-to-beta'); + } else if (await isStackedPr(github, context)) { + // GitHub manages the merge order for a stack, so these are not blocked. + labels.add('stacked-pr'); } else if (baseRef !== 'dev') { + // A chain built by hand: it must not merge until its base branch does. labels.add('chained-pr'); } diff --git a/.github/scripts/auto-label-pr/index.js b/.github/scripts/auto-label-pr/index.js index c8bdcfb2f38..8b0c8215031 100644 --- a/.github/scripts/auto-label-pr/index.js +++ b/.github/scripts/auto-label-pr/index.js @@ -88,7 +88,7 @@ module.exports = async ({ github, context }) => { // Early exit for release and beta branches only if (baseRef === 'release' || baseRef === 'beta') { - const branchLabels = await detectMergeBranch(context); + const branchLabels = await detectMergeBranch(github, context); const finalLabels = Array.from(branchLabels); console.log('Computed labels (merge branch only):', finalLabels.join(', ')); @@ -118,7 +118,7 @@ module.exports = async ({ github, context }) => { deprecatedResult, maintainerAccess ] = await Promise.all([ - detectMergeBranch(context), + detectMergeBranch(github, context), detectComponentPlatforms(changedFiles, apiData), detectNewComponents(github, context, prFiles), detectNewPlatforms(github, context, prFiles, apiData), diff --git a/.github/scripts/auto-label-pr/tests/detectors.test.js b/.github/scripts/auto-label-pr/tests/detectors.test.js index 413fdb3f945..f30ceff8c11 100644 --- a/.github/scripts/auto-label-pr/tests/detectors.test.js +++ b/.github/scripts/auto-label-pr/tests/detectors.test.js @@ -1,6 +1,7 @@ const { describe, it } = require('node:test'); const assert = require('node:assert/strict'); const { + detectMergeBranch, detectNewPlatforms, detectNewComponents, detectPRSize, @@ -36,6 +37,107 @@ const API_DATA = { const WITH_SCHEMA = 'CONFIG_SCHEMA = cv.Schema({})'; const WITHOUT_SCHEMA = 'CODEOWNERS = ["@esphome/core"]'; +// --------------------------------------------------------------------------- +// detectMergeBranch +// --------------------------------------------------------------------------- + +// Builds a fresh context for detectMergeBranch tests instead of mutating the +// shared CONTEXT fixture above (which other describe blocks rely on). +function makeMergeContext(baseRef, { stack } = {}) { + const pull_request = { number: 1, base: { ref: baseRef } }; + if (stack !== undefined) { + pull_request.stack = stack; + } + return { + repo: { owner: 'esphome', repo: 'esphome' }, + payload: { pull_request } + }; +} + +// A GitHub API mock exposing only rest.pulls.get, with a call counter so +// tests can assert whether the API fallback was actually invoked. +function makeStackGithub({ stack = null, error = null } = {}) { + const state = { calls: 0 }; + const github = { + rest: { + pulls: { + get: async () => { + state.calls++; + if (error) throw error; + return { data: { stack } }; + } + } + } + }; + return { github, state }; +} + +const STACK_INFO = { base: { ref: 'dev' }, id: 71540, number: 17978, position: 3, size: 3 }; + +describe('detectMergeBranch', () => { + it('base ref release adds merging-to-release only and never checks the stack', async () => { + const { github, state } = makeStackGithub({ stack: STACK_INFO }); + const context = makeMergeContext('release', { stack: STACK_INFO }); + const labels = await detectMergeBranch(github, context); + assert.deepEqual(Array.from(labels).sort(), ['merging-to-release']); + assert.equal(state.calls, 0); + }); + + it('base ref beta adds merging-to-beta only and never checks the stack', async () => { + const { github, state } = makeStackGithub({ stack: STACK_INFO }); + const context = makeMergeContext('beta', { stack: STACK_INFO }); + const labels = await detectMergeBranch(github, context); + assert.deepEqual(Array.from(labels).sort(), ['merging-to-beta']); + assert.equal(state.calls, 0); + }); + + it('stack present on the webhook payload adds stacked-pr without calling the API', async () => { + const { github, state } = makeStackGithub(); + const context = makeMergeContext('feature-branch', { stack: STACK_INFO }); + const labels = await detectMergeBranch(github, context); + assert.deepEqual(Array.from(labels).sort(), ['stacked-pr']); + assert.equal(state.calls, 0); + }); + + it('stack absent from payload falls back to the API and adds stacked-pr', async () => { + const { github, state } = makeStackGithub({ stack: STACK_INFO }); + const context = makeMergeContext('feature-branch'); + const labels = await detectMergeBranch(github, context); + assert.deepEqual(Array.from(labels).sort(), ['stacked-pr']); + assert.equal(state.calls, 1); + }); + + it('bottom of a stack (base ref dev, stack present) still adds stacked-pr', async () => { + const { github, state } = makeStackGithub(); + const context = makeMergeContext('dev', { stack: STACK_INFO }); + const labels = await detectMergeBranch(github, context); + assert.deepEqual(Array.from(labels).sort(), ['stacked-pr']); + assert.equal(state.calls, 0); + }); + + it('not stacked, base ref not dev adds chained-pr', async () => { + const { github } = makeStackGithub({ stack: null }); + const context = makeMergeContext('feature-branch'); + const labels = await detectMergeBranch(github, context); + assert.deepEqual(Array.from(labels).sort(), ['chained-pr']); + }); + + it('not stacked, base ref dev adds no labels', async () => { + const { github } = makeStackGithub({ stack: null }); + const context = makeMergeContext('dev'); + const labels = await detectMergeBranch(github, context); + assert.deepEqual(Array.from(labels).sort(), []); + }); + + it('a failed stack lookup falls back to not-stacked, so a feature-branch base adds chained-pr', async () => { + const { github, state } = makeStackGithub({ error: new Error('API unavailable') }); + const context = makeMergeContext('feature-branch'); + const labels = await detectMergeBranch(github, context); + assert.deepEqual(Array.from(labels).sort(), ['chained-pr']); + assert.equal(state.calls, 1); + }); +}); + // --------------------------------------------------------------------------- // detectNewPlatforms // --------------------------------------------------------------------------- diff --git a/.github/workflows/auto-label-pr.yml b/.github/workflows/auto-label-pr.yml index 30915b68c78..b49c66b976d 100644 --- a/.github/workflows/auto-label-pr.yml +++ b/.github/workflows/auto-label-pr.yml @@ -35,7 +35,7 @@ jobs: # Scope the minted App token to the minimum needed by auto-label-pr/*.js. permission-contents: read # repos.getContent for CODEOWNERS and file lookups in detectors.js permission-issues: write # listLabelsOnIssue, addLabels, removeLabel, list/createComment - permission-pull-requests: write # pulls.listFiles, list/create/update/dismissReview + permission-pull-requests: write # pulls.get, pulls.listFiles, list/create/update/dismissReview - name: Auto Label PR uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0