From f6cb23ce2a0f96ee1d9d6323976a24dea86bf8c8 Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Tue, 18 Aug 2026 17:38:18 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=94=A7=EF=BC=9Astop?= =?UTF-8?q?=20the=20queue=20citing=20its=20own=20refusal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A refusal exits non-zero, so an attempt made while a check was still running leaves a failed check named Land on the commit. The next attempt read that as a check that did not pass, and refused. Every attempt after the first cited the first, so a pull request labelled a moment too early could never be landed from that commit again. The queue already left its own run out, but by run id, which only matches the run doing the asking. The workflow now also passes the name it reports as, and any run under that name is left out whether it is this one or one from before. Two tests: the refusal it left behind is ignored, and a failure that is not the queue's is still reported. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- .github/workflows/commit-queue.yml | 4 ++++ build/shared/landing.mts | 27 +++++++++++++---------- build/shared/landing.test.mts | 35 ++++++++++++++++++++++++++++++ build/tasks/land-pull-request.mts | 3 ++- 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/.github/workflows/commit-queue.yml b/.github/workflows/commit-queue.yml index f9db4b0be..3203b1bfa 100644 --- a/.github/workflows/commit-queue.yml +++ b/.github/workflows/commit-queue.yml @@ -64,6 +64,10 @@ jobs: # anything is merged. Applying a label needs only triage. LAND_ACTOR: ${{ github.event.sender.login }} NUMBER: ${{ github.event.pull_request.number }} + # What this job reports as, which is the `name:` above. A refusal + # exits non-zero and leaves a failed check behind, so without this + # the first one would be cited by every attempt after it. + LAND_CHECK_NAME: Land run: node build/tasks/land-pull-request.mts "${NUMBER}" # The label is a request, not a state: once the queue has answered it, diff --git a/build/shared/landing.mts b/build/shared/landing.mts index dcb3dd5cc..d32032f45 100644 --- a/build/shared/landing.mts +++ b/build/shared/landing.mts @@ -107,25 +107,30 @@ export type CommitStatus = { context: string; state: string }; * 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. - * The queue is itself a check, so its own run is left out. Waiting for it - * would be waiting for a job that cannot finish until it stops waiting. - * @param {CheckRun[]} runs The check runs reported on the commit. + * + * The queue is itself a check, and none of its runs count -- neither the one + * doing the asking, which cannot finish until it stops waiting, nor any run + * before it. A refusal exits non-zero and so leaves a failed check of its own + * behind, and counting that would let the first refusal decide every later + * one: label a pull request while a check is in flight, and the queue would + * go on citing its own complaint for as long as the branch sat at that commit. + * @param {CheckRun[]} all The check runs reported on the commit. * @param {CommitStatus[]} statuses The commit statuses reported on it. * @param {string} ownRunId The workflow run doing the asking, if it is one. + * @param {string} ownCheckName What the queue reports as, if it knows. * @returns {string} The reason, or an empty string if there is none. */ export function checksVerdict( all: CheckRun[], statuses: CommitStatus[], - ownRunId = '' + ownRunId = '', + ownCheckName = '' ) { - const runs = - ownRunId === '' - ? all - : all.filter( - (run) => - !(run.detailsUrl ?? '').includes(`/actions/runs/${ownRunId}/`) - ); + const isOwn = (run: CheckRun) => + (ownRunId !== '' && + (run.detailsUrl ?? '').includes(`/actions/runs/${ownRunId}/`)) || + (ownCheckName !== '' && run.name === ownCheckName); + const runs = all.filter((run) => !isOwn(run)); const pending = [ ...runs.filter((run) => run.status !== 'completed').map((run) => run.name), ...statuses diff --git a/build/shared/landing.test.mts b/build/shared/landing.test.mts index 0e3fcf5b1..dbed9cd3c 100644 --- a/build/shared/landing.test.mts +++ b/build/shared/landing.test.mts @@ -273,4 +273,39 @@ describe('checksVerdict and its own run', () => { match(checksVerdict([other], [], '999'), /have not finished: Lint/); }); + + test('does not cite the refusal it left behind last time', () => { + // A refusal exits non-zero, so an attempt made while a check was still + // running leaves a failed check of its own on the commit. Counting it + // would mean the first refusal decided every later one, and the pull + // request could never be landed from that commit again. + const earlier = { + name: 'Land', + status: 'completed', + conclusion: 'failure', + detailsUrl: 'https://github.com/o/r/actions/runs/998/job/1', + }; + + deepStrictEqual( + checksVerdict( + [earlier, { name: 'Lint', status: 'completed', conclusion: 'success' }], + [], + '999', + 'Land' + ), + '' + ); + }); + + test('still reports a failure that is not the queue', () => { + match( + checksVerdict( + [{ name: 'Lint', status: 'completed', conclusion: 'failure' }], + [], + '999', + 'Land' + ), + /did not pass: Lint/ + ); + }); }); diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index 5e2f8819e..92f3479ce 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -157,7 +157,8 @@ const checksRefuse = (sha: string) => '[.statuses[] | {context, state}]' ) ), - process.env.GITHUB_RUN_ID ?? '' + process.env.GITHUB_RUN_ID ?? '', + process.env.LAND_CHECK_NAME ?? '' ); /**