From c5ba70f67e02840d2e8ff3cdf7896d689347e853 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 12:24:17 +0200 Subject: [PATCH] ci(release): let a later green ci-ok supersede a red run on the same commit Both release workflows refused a commit if any ci-ok check run on it was not green. Every workflow run is its own check suite, so a red run stays on the commit after a later one passed: a flaky job on the dev push, or a push run cancelled by a manual run on the same ref (ci-ok runs with always() and fails on the cancelled jobs). Running Tests & SonarQube again, as the error told the maintainer to do, added a green suite but never cleared the red one, and the message did not say which run was red. The gate now lives in scripts/ci/ci-ok-verdict.mjs. It fetches every attempt (filter=all), keeps the newest attempt per check suite, refuses while one is still going and requires the run that finished last to be green. It prints the link of every run it counted, and an older red run it overrode shows up as a notice. A response it cannot read, one cut off by pagination or a run without a suite, conclusion or completion time fails the release instead of passing it. Tests run with test:ci-scripts. --- .github/workflows/docker-dev.yml | 23 ++--- .github/workflows/docker.yml | 24 ++--- scripts/ci/ci-ok-verdict.mjs | 113 +++++++++++++++++++++ scripts/ci/ci-ok-verdict.test.mjs | 160 ++++++++++++++++++++++++++++++ 4 files changed, 289 insertions(+), 31 deletions(-) create mode 100644 scripts/ci/ci-ok-verdict.mjs create mode 100644 scripts/ci/ci-ok-verdict.test.mjs diff --git a/.github/workflows/docker-dev.yml b/.github/workflows/docker-dev.yml index ccda72479..594da243b 100644 --- a/.github/workflows/docker-dev.yml +++ b/.github/workflows/docker-dev.yml @@ -37,27 +37,20 @@ jobs: fetch-depth: 0 token: ${{ secrets.GITHUB_TOKEN }} - # Every check run named ci-ok on this commit has to have finished green. - # A run still going counts as not green: wait for it and dispatch again. + # The dev push, the main push and any manual run each leave a ci-ok on + # this commit. scripts/ci/ci-ok-verdict.mjs counts the newest attempt of + # each, refuses while one is still going, and requires the one that + # finished last to be green, so a later green run supersedes a flaky or + # cancelled one. It prints the link of every run it counted. - name: Require a green ci-ok on the commit being built env: GH_TOKEN: ${{ github.token }} run: | set -euo pipefail sha="$(git rev-parse HEAD)" - conclusions="$(gh api "repos/$GITHUB_REPOSITORY/commits/$sha/check-runs?check_name=ci-ok&per_page=100" \ - --jq '[.check_runs[] | .conclusion // "pending"] | join(" ")')" - echo "ci-ok on $sha: ${conclusions:-none}" - if [ -z "$conclusions" ]; then - echo "::error::ci-ok never ran on $sha. Run Tests & SonarQube on this branch and dispatch again." - exit 1 - fi - for c in $conclusions; do - if [ "$c" != "success" ]; then - echo "::error::ci-ok on $sha is not green ($conclusions)." - exit 1 - fi - done + runs="$RUNNER_TEMP/ci-ok-runs.json" + gh api "repos/$GITHUB_REPOSITORY/commits/$sha/check-runs?check_name=ci-ok&filter=all&per_page=100" > "$runs" + node scripts/ci/ci-ok-verdict.mjs "$runs" "$sha" - name: Determine prerelease version id: bump diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 1c038200a..a57020e31 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -62,28 +62,20 @@ jobs: fetch-tags: true token: ${{ steps.app-token.outputs.token }} - # Every check run named ci-ok on this commit (the dev push and the main - # push each leave one) has to have finished green. A run still going - # counts as not green: wait for it and dispatch again. + # The dev push, the main push and any manual run each leave a ci-ok on + # this commit. scripts/ci/ci-ok-verdict.mjs counts the newest attempt of + # each, refuses while one is still going, and requires the one that + # finished last to be green, so a later green run supersedes a flaky or + # cancelled one. It prints the link of every run it counted. - name: Require a green ci-ok on the commit being released env: GH_TOKEN: ${{ github.token }} run: | set -euo pipefail sha="$(git rev-parse HEAD)" - conclusions="$(gh api "repos/$GITHUB_REPOSITORY/commits/$sha/check-runs?check_name=ci-ok&per_page=100" \ - --jq '[.check_runs[] | .conclusion // "pending"] | join(" ")')" - echo "ci-ok on $sha: ${conclusions:-none}" - if [ -z "$conclusions" ]; then - echo "::error::ci-ok never ran on $sha. Run Tests & SonarQube on main and dispatch again." - exit 1 - fi - for c in $conclusions; do - if [ "$c" != "success" ]; then - echo "::error::ci-ok on $sha is not green ($conclusions)." - exit 1 - fi - done + runs="$RUNNER_TEMP/ci-ok-runs.json" + gh api "repos/$GITHUB_REPOSITORY/commits/$sha/check-runs?check_name=ci-ok&filter=all&per_page=100" > "$runs" + node scripts/ci/ci-ok-verdict.mjs "$runs" "$sha" - name: Determine bump type and update version id: bump diff --git a/scripts/ci/ci-ok-verdict.mjs b/scripts/ci/ci-ok-verdict.mjs new file mode 100644 index 000000000..ab8f0776e --- /dev/null +++ b/scripts/ci/ci-ok-verdict.mjs @@ -0,0 +1,113 @@ +#!/usr/bin/env node +// Decides whether a commit has a green ci-ok, for the release workflows. +// +// docker.yml and docker-dev.yml refuse to build a commit unless ci-ok passed +// on it. They fetch every ci-ok check run on the commit, earlier attempts +// included (GET /repos/{repo}/commits/{sha}/check-runs?check_name=ci-ok&filter=all), +// and hand the response to this script. +// +// A commit can carry several ci-ok runs: the dev push, the main push and any +// manual dispatch each start their own workflow run, and each workflow run is +// its own check suite. A red suite stays on the commit after a later one passed +// (a flaky job, a push run cancelled by a dispatch on the same ref), so +// requiring every run to be green would block the release until someone found +// and re-ran that exact old run. Instead: +// +// 1. Per check suite only the newest check run counts. Re-running a workflow +// run adds a check run to the same suite, and that one replaces the old. +// 2. A run still going fails the gate: wait for it and dispatch again. +// 3. Of the rest, the one that completed last decides. It has to be green. +// +// Usage: +// node scripts/ci/ci-ok-verdict.mjs +// +// Prints one line per counted run and exits 0 only when the gate passes. It +// fails closed: a file it cannot read or parse, a response cut off by +// pagination, or a run without the fields it needs is an error, never a pass. + +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +function requireField(run, value, name) { + if (value === undefined || value === null || value === '') { + throw new Error(`check run ${run?.id ?? '?'} has no ${name}`); + } + return value; +} + +// Parses the API response into the runs that count, newest per check suite. +export function latestPerSuite(response) { + if (!response || !Array.isArray(response.check_runs)) { + throw new Error('response has no check_runs array'); + } + if (typeof response.total_count === 'number' && response.total_count > response.check_runs.length) { + throw new Error(`response lists ${response.check_runs.length} of ${response.total_count} check runs`); + } + const bySuite = new Map(); + for (const run of response.check_runs) { + const id = requireField(run, run?.id, 'id'); + const suite = requireField(run, run.check_suite?.id, 'check_suite.id'); + const status = requireField(run, run.status, 'status'); + if (status === 'completed') { + requireField(run, run.conclusion, 'conclusion'); + if (Number.isNaN(Date.parse(requireField(run, run.completed_at, 'completed_at')))) { + throw new Error(`check run ${id} has an unreadable completed_at`); + } + } + const kept = bySuite.get(suite); + if (!kept || id > kept.id) bySuite.set(suite, run); + } + return [...bySuite.values()]; +} + +// Returns { ok, lines } for the API response of one commit. +export function verdict(response, sha) { + const runs = latestPerSuite(response); + const describe = (run) => + `${run.status === 'completed' ? run.conclusion : run.status} ${run.html_url ?? `check run ${run.id}`}`; + + if (runs.length === 0) { + return { ok: false, lines: [`::error::ci-ok never ran on ${sha}. Run Tests & SonarQube on it and dispatch again.`] }; + } + + const lines = runs.map((run) => `ci-ok on ${sha}: ${describe(run)}`); + const pending = runs.filter((run) => run.status !== 'completed'); + if (pending.length > 0) { + for (const run of pending) lines.push(`::error::ci-ok on ${sha} is still running: ${describe(run)}. Wait for it and dispatch again.`); + return { ok: false, lines }; + } + + const newest = runs.reduce((a, b) => { + const diff = Date.parse(b.completed_at) - Date.parse(a.completed_at); + return diff > 0 || (diff === 0 && b.id > a.id) ? b : a; + }); + if (newest.conclusion !== 'success') { + lines.push(`::error::The newest ci-ok on ${sha} is not green: ${describe(newest)}. Re-run that run, or run Tests & SonarQube on it again, then dispatch again.`); + return { ok: false, lines }; + } + for (const run of runs) { + if (run !== newest && run.conclusion !== 'success') { + lines.push(`::notice::An older ci-ok on ${sha} is ${run.conclusion} (${run.html_url ?? run.id}); the newer green run supersedes it.`); + } + } + return { ok: true, lines }; +} + +function main() { + const args = process.argv.slice(2); + if (args.length !== 2 || !args[0] || !args[1]) throw new Error('usage: ci-ok-verdict.mjs '); + // An unreadable or malformed file throws and fails the job. + const response = JSON.parse(readFileSync(args[0], 'utf8')); + const result = verdict(response, args[1]); + console.log(result.lines.join('\n')); + if (!result.ok) process.exit(1); +} + +if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { + try { + main(); + } catch (err) { + console.error(`::error::${err instanceof Error ? err.message : err}`); + process.exit(1); + } +} diff --git a/scripts/ci/ci-ok-verdict.test.mjs b/scripts/ci/ci-ok-verdict.test.mjs new file mode 100644 index 000000000..f5adb7c3b --- /dev/null +++ b/scripts/ci/ci-ok-verdict.test.mjs @@ -0,0 +1,160 @@ +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { after, describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; +import { latestPerSuite, verdict } from './ci-ok-verdict.mjs'; + +const script = fileURLToPath(new URL('./ci-ok-verdict.mjs', import.meta.url)); +const scratch = mkdtempSync(join(tmpdir(), 'ci-ok-verdict-')); +after(() => rmSync(scratch, { recursive: true, force: true })); + +const SHA = 'abc123'; + +function run(id, suite, conclusion, completedAt, status = 'completed') { + return { + id, + status, + conclusion: status === 'completed' ? conclusion : null, + completed_at: status === 'completed' ? completedAt : null, + html_url: `https://github.com/liketrek/TREK/actions/runs/${suite}/job/${id}`, + check_suite: { id: suite }, + }; +} + +function response(...runs) { + return { total_count: runs.length, check_runs: runs }; +} + +function cli(content) { + const file = join(scratch, `runs-${Math.random().toString(36).slice(2)}.json`); + if (content !== undefined) writeFileSync(file, content); + return spawnSync(process.execPath, [script, file, SHA], { encoding: 'utf8' }); +} + +describe('verdict', () => { + it('passes a single green run', () => { + assert.equal(verdict(response(run(1, 10, 'success', '2026-10-01T10:00:00Z')), SHA).ok, true); + }); + + it('fails when ci-ok never ran', () => { + const result = verdict(response(), SHA); + assert.equal(result.ok, false); + assert.match(result.lines.join('\n'), /never ran/); + }); + + it('fails when the newest run is red and names it', () => { + const result = verdict( + response(run(1, 10, 'success', '2026-10-01T10:00:00Z'), run(2, 20, 'failure', '2026-10-01T11:00:00Z')), + SHA, + ); + assert.equal(result.ok, false); + assert.match(result.lines.join('\n'), /not green: failure https:\/\/github\.com\/.*\/runs\/20\/job\/2/); + }); + + it('lets a later green suite supersede a flaky or cancelled one', () => { + const result = verdict( + response( + run(1, 10, 'failure', '2026-10-01T10:00:00Z'), + run(2, 20, 'cancelled', '2026-10-01T10:30:00Z'), + run(3, 30, 'success', '2026-10-01T11:00:00Z'), + ), + SHA, + ); + assert.equal(result.ok, true); + assert.match(result.lines.join('\n'), /older ci-ok .* is failure/); + }); + + it('counts a re-run attempt in place of the attempt it replaced', () => { + // Same suite: the re-run (higher id) replaces the red attempt, even though + // the red one would otherwise be the newest suite result. + const result = verdict( + response(run(1, 10, 'success', '2026-10-01T09:00:00Z'), run(2, 20, 'failure', '2026-10-01T10:00:00Z'), run(5, 20, 'success', '2026-10-01T12:00:00Z')), + SHA, + ); + assert.equal(result.ok, true); + assert.equal(latestPerSuite(response(run(2, 20, 'failure', '2026-10-01T10:00:00Z'), run(5, 20, 'success', '2026-10-01T12:00:00Z'))).length, 1); + }); + + it('fails when a re-run of the newest suite turned red', () => { + const result = verdict( + response(run(2, 20, 'success', '2026-10-01T10:00:00Z'), run(5, 20, 'failure', '2026-10-01T12:00:00Z')), + SHA, + ); + assert.equal(result.ok, false); + }); + + it('fails while any counted run is still going', () => { + const result = verdict( + response(run(1, 10, 'success', '2026-10-01T10:00:00Z'), run(2, 20, null, null, 'in_progress')), + SHA, + ); + assert.equal(result.ok, false); + assert.match(result.lines.join('\n'), /still running: in_progress/); + }); + + it('treats skipped or neutral as not green', () => { + assert.equal(verdict(response(run(1, 10, 'skipped', '2026-10-01T10:00:00Z')), SHA).ok, false); + assert.equal(verdict(response(run(1, 10, 'neutral', '2026-10-01T10:00:00Z')), SHA).ok, false); + }); + + it('breaks a completion-time tie by the higher id', () => { + const at = '2026-10-01T10:00:00Z'; + assert.equal(verdict(response(run(1, 10, 'success', at), run(2, 20, 'failure', at)), SHA).ok, false); + assert.equal(verdict(response(run(1, 10, 'failure', at), run(2, 20, 'success', at)), SHA).ok, true); + }); +}); + +describe('fails closed', () => { + it('rejects a response without check_runs', () => { + assert.throws(() => verdict({ total_count: 0 }, SHA), /no check_runs/); + }); + + it('rejects a response cut off by pagination', () => { + assert.throws( + () => verdict({ total_count: 101, check_runs: [run(1, 10, 'success', '2026-10-01T10:00:00Z')] }, SHA), + /1 of 101/, + ); + }); + + it('rejects a run without a check suite, conclusion or completion time', () => { + const noSuite = { ...run(1, 10, 'success', '2026-10-01T10:00:00Z'), check_suite: null }; + assert.throws(() => verdict(response(noSuite), SHA), /check_suite\.id/); + const noConclusion = { ...run(1, 10, 'success', '2026-10-01T10:00:00Z'), conclusion: null }; + assert.throws(() => verdict(response(noConclusion), SHA), /conclusion/); + const badTime = run(1, 10, 'success', 'yesterday'); + assert.throws(() => verdict(response(badTime), SHA), /completed_at/); + }); +}); + +describe('cli', () => { + it('exits 0 on a green commit', () => { + const result = cli(JSON.stringify(response(run(1, 10, 'success', '2026-10-01T10:00:00Z')))); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /ci-ok on abc123: success/); + }); + + it('exits 1 on a red commit', () => { + const result = cli(JSON.stringify(response(run(1, 10, 'failure', '2026-10-01T10:00:00Z')))); + assert.equal(result.status, 1); + assert.match(result.stdout, /::error::/); + }); + + it('exits 1 on a missing file, broken JSON or wrong arguments', () => { + assert.equal(cli(undefined).status, 1); + assert.equal(cli('{not json').status, 1); + assert.equal(spawnSync(process.execPath, [script], { encoding: 'utf8' }).status, 1); + }); +}); + +describe('release workflows', () => { + for (const name of ['docker.yml', 'docker-dev.yml']) { + it(`${name} gates on this script`, () => { + const workflow = readFileSync(fileURLToPath(new URL(`../../.github/workflows/${name}`, import.meta.url)), 'utf8'); + assert.match(workflow, /check-runs\?check_name=ci-ok&filter=all&per_page=100/); + assert.match(workflow, /node scripts\/ci\/ci-ok-verdict\.mjs "\$runs" "\$sha"/); + }); + } +});