From 770f08bcf437c71efa783a6305a6ea2314f34a1d Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Sat, 15 Aug 2026 03:20:33 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=E2=9C=A8=EF=BC=9Aland=20a?= =?UTF-8?q?=20pull=20request=20when=20it=20is=20labelled?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The commit queue. Applying `commit-queue` to a pull request hands it to the same task that has been landing them by hand, running as a GitHub app rather than as anyone in particular. Two things are checked before anything merges, beyond what landing by hand already checked. Every check has to have finished and passed, since a label applied while one was in flight says nothing about how it turned out. And whoever applied the label has to have the right to push: labelling needs only triage, so without this the label would quietly hand out an access level GitHub had withheld. Nothing here checks out or runs the branch's code. `pull_request_target` reaches secrets, which is why it is used, and that is only safe while the code being read belongs to the base branch. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- .github/workflows/commit-queue.yml | 76 +++++++++++++++++++++++++++++ build/shared/landing.mts | 49 +++++++++++++++++++ build/shared/landing.test.mts | 66 +++++++++++++++++++++++++ build/tasks/land-pull-request.mts | 78 ++++++++++++++++++++++++++++-- 4 files changed, 265 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/commit-queue.yml diff --git a/.github/workflows/commit-queue.yml b/.github/workflows/commit-queue.yml new file mode 100644 index 000000000..3c13dd059 --- /dev/null +++ b/.github/workflows/commit-queue.yml @@ -0,0 +1,76 @@ +# Landing a pull request when it is labelled `commit-queue`. +# +# `pull_request_target` runs in the context of the base branch and can reach +# secrets, which `pull_request` cannot do for a fork. That is only safe +# because nothing here checks out or executes the pull request's code: the +# checkout below is the base branch, and the branch under review is fetched +# only so that its commit messages can be read. Never add a build, an install +# or a test step to this workflow -- those belong in the checks it waits for, +# which run without a token that can write anything. +# +# Actions are pinned by commit, never by tag. +name: Commit Queue + +on: + pull_request_target: + types: [labeled] + +permissions: + contents: read + +# Two labels applied in quick succession should not race each other into the +# same merge. +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: false + +jobs: + land: + name: Land + if: github.event.label.name == 'commit-queue' + runs-on: ubuntu-latest + steps: + - name: Mint a token for the app + id: token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ secrets.LAND_APP_ID }} + private-key: ${{ secrets.LAND_APP_PRIVATE_KEY }} + + - name: Check out the base branch + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # The base, deliberately, not the pull request. Full history so that + # the range between the two can be read. + ref: ${{ github.event.pull_request.base.ref }} + fetch-depth: 0 + + - name: Set up Node.js runtime + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version-file: 'package.json' + + - name: Fetch the commits under review + env: + NUMBER: ${{ github.event.pull_request.number }} + run: git fetch --quiet origin "pull/${NUMBER}/head" + + - name: Land it + id: land + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + # Whoever applied the label, whose right to push is checked before + # anything is merged. Applying a label needs only triage. + LAND_ACTOR: ${{ github.event.sender.login }} + NUMBER: ${{ github.event.pull_request.number }} + run: node build/tasks/land-pull-request.mts "${NUMBER}" + + - name: Say why it did not land + if: failure() + env: + GH_TOKEN: ${{ steps.token.outputs.token }} + NUMBER: ${{ github.event.pull_request.number }} + RUN: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + gh pr edit "$NUMBER" --remove-label commit-queue + gh pr comment "$NUMBER" --body "The commit queue did not land this. See $RUN — the label has been removed, so re-apply it once the reason is dealt with." diff --git a/build/shared/landing.mts b/build/shared/landing.mts index 1b3d9ede5..3f83f43bc 100644 --- a/build/shared/landing.mts +++ b/build/shared/landing.mts @@ -92,3 +92,52 @@ export function composeLandingMessage(parts: CommitParts[], prUrl: string) { ), ].join('\n'); } + +/** What GitHub reports about one check run on a commit. */ +export type CheckRun = { + name: string; + status: string; + conclusion: string | null; +}; + +/** What GitHub reports about one commit status, which is the older kind. */ +export type CommitStatus = { context: string; state: string }; + +/** + * Says why the checks on a commit are not a reason to land it. Anything still + * running counts against it: a label applied while a check was in flight says + * nothing about how that check turned out. Neutral and skipped do not count + * against it, since neither is a complaint. + * @param {CheckRun[]} runs The check runs reported on the commit. + * @param {CommitStatus[]} statuses The commit statuses reported on it. + * @returns {string} The reason, or an empty string if there is none. + */ +export function checksVerdict(runs: CheckRun[], statuses: CommitStatus[]) { + const pending = [ + ...runs.filter((run) => run.status !== 'completed').map((run) => run.name), + ...statuses + .filter((status) => status.state === 'pending') + .map((status) => status.context), + ]; + + if (pending.length > 0) { + return `these have not finished: ${pending.sort().join(', ')}`; + } + + const failed = [ + ...runs + .filter( + (run) => + run.conclusion !== null && + !['success', 'neutral', 'skipped'].includes(run.conclusion) + ) + .map((run) => run.name), + ...statuses + .filter((status) => !['success', 'pending'].includes(status.state)) + .map((status) => status.context), + ]; + + return failed.length > 0 + ? `these did not pass: ${failed.sort().join(', ')}` + : ''; +} diff --git a/build/shared/landing.test.mts b/build/shared/landing.test.mts index 0da47c21e..4317d076c 100644 --- a/build/shared/landing.test.mts +++ b/build/shared/landing.test.mts @@ -10,6 +10,7 @@ import { execFileSync } from 'node:child_process'; import { describe, test } from 'node:test'; import { validateCommitMessage } from '@openinf/portal/build/commit-message'; import { + checksVerdict, composeLandingMessage, partsOfMessage, } from '@openinf/portal/build/landing'; @@ -175,3 +176,68 @@ describe('the message that comes out', () => { ); }); }); + +describe('checksVerdict', () => { + const run = ( + name: string, + conclusion: string | null, + status = 'completed' + ) => ({ + name, + status, + conclusion, + }); + + test('says nothing when everything passed', () => { + deepStrictEqual(checksVerdict([run('Lint and test', 'success')], []), ''); + }); + + test('counts anything still running against it', () => { + // A label applied while a check was in flight says nothing about how that + // check turned out. + match( + checksVerdict([run('Lint and test', null, 'in_progress')], []), + /have not finished: Lint and test/ + ); + }); + + test('reports a failure by name', () => { + match( + checksVerdict([run('CodeQL', 'failure')], []), + /did not pass: CodeQL/ + ); + }); + + test('treats neutral and skipped as no complaint', () => { + deepStrictEqual( + checksVerdict( + [run('Pages changed', 'neutral'), run('Deploy', 'skipped')], + [] + ), + '' + ); + }); + + test('reads the older commit statuses too', () => { + // Some services on this repository report these rather than check runs, + // so looking only at check runs would call a red commit green. + match( + checksVerdict([], [{ context: 'ci/legacy', state: 'failure' }]), + /did not pass: ci\/legacy/ + ); + }); + + test('holds a pending status back as well', () => { + match( + checksVerdict([], [{ context: 'ci/legacy', state: 'pending' }]), + /have not finished: ci\/legacy/ + ); + }); + + test('reports everything wrong, not just the first', () => { + match( + checksVerdict([run('A', 'failure'), run('B', 'timed_out')], []), + /did not pass: A, B/ + ); + }); +}); diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index 8941b58ed..f2d7a83b8 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -14,6 +14,7 @@ import { execFileSync } from 'node:child_process'; import { validateCommitMessage } from '@openinf/portal/build/commit-message'; import { + checksVerdict, composeLandingMessage, partsOfMessage, } from '@openinf/portal/build/landing'; @@ -28,9 +29,6 @@ type PullRequest = { mergeableState: string; }; -const REPOSITORY = 'OpenINF/openinf.github.io'; -const DEFAULT_BRANCH = 'live'; - const [number, ...flags] = process.argv.slice(2); const dryRun = flags.includes('--dry-run'); @@ -39,6 +37,26 @@ const gh = (...args: string[]) => const git = (...args: string[]) => execFileSync('git', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); +// Nothing here names a repository. A workflow sets GITHUB_REPOSITORY, and a +// terminal has a remote to read it from, so a copy of this file lands in +// another repository without being edited first. +const REPOSITORY = + process.env.GITHUB_REPOSITORY ?? + git('remote', 'get-url', 'origin').match( + /github\.com[/:](?[^/]+\/[^/]+?)(?:\.git)?$/ + )?.groups?.repo ?? + ''; + +const DEFAULT_BRANCH = gh( + 'api', + `repos/${REPOSITORY}`, + '--jq', + '.default_branch' +); + +/** Who is asking for this to land: the labeller, or whoever is at the keyboard. */ +const ACTOR = process.env.LAND_ACTOR ?? gh('api', 'user', '--jq', '.login'); + /** * Reads the pull request, waiting for GitHub to work out whether it merges. * The answer is `unknown` for a moment after anything lands on the base, and @@ -75,6 +93,52 @@ const readPull = async (pull: string): Promise => { return found; }; +/** + * Fetches what GitHub reports about a commit, and asks whether it is a reason + * to land. Both kinds are read: some services on this repository still report + * the older commit statuses rather than check runs. + * @param {string} sha The commit the pull request is at. + * @returns {string} The reason not to land, or an empty string if there is none. + */ +const checksRefuse = (sha: string) => + checksVerdict( + JSON.parse( + gh( + 'api', + `repos/${REPOSITORY}/commits/${sha}/check-runs`, + '--jq', + '[.check_runs[] | {name, status, conclusion}]' + ) + ), + JSON.parse( + gh( + 'api', + `repos/${REPOSITORY}/commits/${sha}/status`, + '--jq', + '[.statuses[] | {context, state}]' + ) + ) + ); + +/** + * Says why the person asking is not entitled to land anything. Applying a + * label needs only triage, which does not carry the right to push -- so + * without this, the label would quietly hand out that right. + * @returns {string} The reason, or an empty string if there is none. + */ +const actorRefuses = () => { + const permission = gh( + 'api', + `repos/${REPOSITORY}/collaborators/${ACTOR}/permission`, + '--jq', + '.permission' + ); + + return ['admin', 'maintain', 'write'].includes(permission) + ? '' + : `${ACTOR} has ${permission} access, which does not carry the right to push`; +}; + /** * Says why a pull request cannot be landed as it stands. * @param {PullRequest} pull What GitHub reports about it. @@ -97,7 +161,13 @@ const refuse = (pull: PullRequest) => { return 'GitHub has not worked out whether it merges yet; try again shortly'; } - return ''; + const actor = actorRefuses(); + + if (actor !== '') return actor; + + const checks = checksRefuse(pull.head); + + return checks === '' ? '' : `${checks}`; }; if (number === undefined || !/^\d+$/.test(number)) {