diff --git a/.github/workflows/ci-triage.yml b/.github/workflows/ci-triage.yml index de74d1bf17..fdab54f2df 100644 --- a/.github/workflows/ci-triage.yml +++ b/.github/workflows/ci-triage.yml @@ -50,6 +50,7 @@ jobs: const { buildTriageBody, classifyFailedJobs, + executedJobNames, failedJobNames, selectLatestDefaultBranchRun, } = await import(moduleUrl); @@ -80,6 +81,9 @@ jobs: // accidentally select a different workflow and misattribute a PR failure. let mainRun; let mainFailingJobs = []; + // `undefined` (not []) until the baseline's jobs are read: an empty list would claim + // the baseline ran nothing, which is a different statement from "not established". + let mainExecutedJobs; try { const { data: mainRuns } = await github.rest.actions.listWorkflowRuns({ owner: context.repo.owner, @@ -94,20 +98,24 @@ jobs: currentRunId: run.id, defaultBranch: context.payload.repository.default_branch, }); - if (mainRun?.conclusion === "failure") { + if (mainRun) { + // Read the baseline's jobs whatever its conclusion. CI is path-scoped, so a green + // main run routinely SKIPS the very job that failed here; without this the comment + // cited that aggregate green as though it covered the failure (ledger #5DYBQQ). const mainJobs = await github.paginate(github.rest.actions.listJobsForWorkflowRun, { owner: context.repo.owner, repo: context.repo.repo, run_id: mainRun.id, per_page: 100, }); - mainFailingJobs = failedJobNames(mainJobs); + mainExecutedJobs = executedJobNames(mainJobs); + if (mainRun.conclusion === "failure") mainFailingJobs = failedJobNames(mainJobs); } } catch (e) { core.info(`main-side check skipped: ${e.message}`); } - const classifications = classifyFailedJobs(failed, mainRun, mainFailingJobs); + const classifications = classifyFailedJobs(failed, mainRun, mainFailingJobs, mainExecutedJobs); const body = buildTriageBody(classifications, mainRun); const marker = ""; diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 884849ba39..788c736988 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,11 +25,23 @@ concurrency: # push to the same ref cannot cancel them mid-flight — release-browser-matrix takes up to # 70 minutes and was repeatedly killed by main churn. PR runs keep the shared ref group # with cancel-in-progress so superseded heads still stop early. - group: ${{ github.workflow }}-${{ (github.event_name == 'workflow_dispatch' || github.event_name == 'schedule') && github.run_id || github.ref }} + # + # Base-branch pushes ALSO get a per-run group, and that is load-bearing rather than cosmetic. + # `cancel-in-progress: false` alone does not protect them: GitHub keeps at most ONE pending run + # per concurrency group, so when merges arrive faster than a run completes, each newly queued + # main run CANCELS the one already waiting. The exemption below was written for supersession and + # never covered queue eviction, so main kept landing unverified anyway — observed 2026-08-20, + # when `a1c2ced`, `d745d15`, `97f6142` and `1cc0d29` were all cancelled while a ~70-minute + # release-browser-matrix held the shared `CI-refs/heads/main` group, and a mobile-/ CLS + # regression rode through the gap to surface on an unrelated PR (#2199) hours later. + # One group per run means a merged commit is never queued behind, or evicted by, another. + group: ${{ github.workflow }}-${{ (github.event_name == 'workflow_dispatch' || github.event_name == 'schedule' || github.event_name == 'push') && github.run_id || github.ref }} # Base-branch pushes are EXEMPT from supersession. `on.push.branches` is [main, release/**], # so `event_name == 'push'` is always a base-branch push — a commit that is already merged and # can never be superseded by a "newer head" the way a PR branch can. Cancelling those runs # does not save redundant work; it destroys the only verification `main` ever gets. + # With the per-run group above this is now belt-and-braces for pushes, and it stays because it + # is the line that states the intent; it remains the only protection if the group ever changes. # # Measured 2026-08-18 over the last 30 pushes to main: 23 cancelled (77%), 6 success, 1 failure. # main was therefore unverified four times out of five, and three separate defects reached a diff --git a/scripts/ci-triage.mjs b/scripts/ci-triage.mjs index 34c94952ec..9f47603a7b 100644 --- a/scripts/ci-triage.mjs +++ b/scripts/ci-triage.mjs @@ -13,6 +13,38 @@ export function failedJobNames(jobs) { ]; } +/** + * Conclusions that mean the job RAN TO A VERDICT, as an allowlist rather than a denylist. + * + * The direction of a mistake is asymmetric, so the list is built to fail in the harmless one. + * Treating a job that did run as unbaselined only says "the comparison is silent here", which + * costs a reader nothing. Treating a job that did NOT run as baselined implies main covered the + * failure — the exact defect this function exists to remove. A denylist inverts that: any GitHub + * conclusion nobody thought to exclude (`timed_out`, `stale`, `action_required`, whatever is added + * next) silently becomes evidence. `cancelled` is the case that matters most here, because a + * cancelled main run is this repo's common failure mode, not a rarity. + */ +const BASELINE_ESTABLISHING_CONCLUSIONS = new Set(["success", "failure", "neutral"]); + +/** + * Job names the baseline run actually EXECUTED — a `skipped` or `cancelled` job verified nothing. + * + * CI is path-scoped, so a docs-only push to main reports `success` with Lighthouse, Production UI + * and Build all skipped. Citing that run as the comparison made the triage comment read as + * "main is green for this job" when main had never measured it: exactly how the mobile-`/` CLS + * regression behind PR #2199 was waved through, and the same trap ledger `#5DYBQQ` records. + */ +export function executedJobNames(jobs) { + return [ + ...new Set( + (jobs ?? []) + .filter((job) => BASELINE_ESTABLISHING_CONCLUSIONS.has(job.conclusion)) + .map((job) => job.name) + .filter(Boolean), + ), + ]; +} + export function selectLatestDefaultBranchRun(runs, { currentRunId, defaultBranch }) { return (runs ?? []) .filter( @@ -29,23 +61,41 @@ export function selectLatestDefaultBranchRun(runs, { currentRunId, defaultBranch )[0]; } -export function classifyFailedJobs(failedNames, mainRun, mainFailedNames) { +export function classifyFailedJobs(failedNames, mainRun, mainFailedNames, mainExecutedNames) { const mainFailures = new Set(mainRun?.conclusion === "failure" ? mainFailedNames : []); - return failedNames.map((name) => ({ - name, - classification: mainFailures.has(name) ? "main-side" : "needs-investigation", - })); + // Omitted (undefined) means the caller could not establish what the baseline ran, so every job + // keeps its previous classification rather than being wrongly reported as unbaselined. + const mainExecuted = mainExecutedNames === undefined ? null : new Set(mainExecutedNames); + return failedNames.map((name) => { + if (mainFailures.has(name)) return { name, classification: "main-side" }; + if (mainRun && mainExecuted && !mainExecuted.has(name)) return { name, classification: "unbaselined" }; + return { name, classification: "needs-investigation" }; + }); } export function buildTriageBody(classifications, mainRun) { const marker = ""; - const lines = classifications.map(({ name, classification }) => - classification === "main-side" - ? `- \`${name}\` — **main-side**: the same job also failed on the latest completed \`main\` CI run.` - : `- \`${name}\` — **needs investigation**: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.`, - ); + const lines = classifications.map(({ name, classification }) => { + if (classification === "main-side") { + return `- \`${name}\` — **main-side**: the same job also failed on the latest completed \`main\` CI run.`; + } + if (classification === "unbaselined") { + return ( + `- \`${name}\` — **not baselined**: this job did NOT run on the \`main\` comparison below ` + + `(path-scoped skip), so that run says nothing about it either way. Treat the comparison as absent, ` + + `not green, and inspect the failing step.` + ); + } + return `- \`${name}\` — **needs investigation**: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.`; + }); + const unbaselined = classifications.filter(({ classification }) => classification === "unbaselined"); const baseline = mainRun - ? `Compared with main CI run [#${mainRun.run_number}](${mainRun.html_url}) (${mainRun.conclusion}).` + ? `Compared with main CI run [#${mainRun.run_number}](${mainRun.html_url}) (${mainRun.conclusion}).` + + (unbaselined.length + ? ` That run's conclusion is an aggregate and did not exercise ${unbaselined + .map(({ name }) => `\`${name}\``) + .join(", ")}.` + : "") : "No completed main CI baseline was available; no failure was labeled main-side."; return [ marker, @@ -76,6 +126,8 @@ function selfTest() { head_branch: "main", status: "completed", conclusion: "success", + run_number: 1234, + html_url: "https://github.com/o/r/actions/runs/1", run_started_at: "2026-07-17T01:00:00Z", }, { @@ -101,6 +153,37 @@ function selfTest() { { name: "Lint", classification: "needs-investigation" }, ]); assert.deepEqual(classifyFailedJobs(["Build"], null, []), [{ name: "Build", classification: "needs-investigation" }]); + assert.deepEqual( + executedJobNames([ + { name: "Build", conclusion: "success" }, + { name: "Failed but ran", conclusion: "failure" }, + { name: "Lighthouse budget", conclusion: "skipped" }, + { name: "Queued", conclusion: null }, + // A cancelled main run is this repo's common failure mode, and a cancelled job verified + // nothing — it must not establish a baseline any more than a skipped one does. + { name: "Production UI", conclusion: "cancelled" }, + { name: "Timed out", conclusion: "timed_out" }, + { name: "Stale", conclusion: "stale" }, + { name: "Action required", conclusion: "action_required" }, + ]), + ["Build", "Failed but ran"], + ); + // The whole point of the allowlist: an unrecognised future conclusion stays out. + assert.deepEqual(executedJobNames([{ name: "Build", conclusion: "some_new_github_state" }]), []); + // A cancelled baseline job therefore reports unbaselined rather than needs-investigation. + assert.deepEqual( + classifyFailedJobs(["Build"], runs[1], [], executedJobNames([{ name: "Build", conclusion: "cancelled" }])), + [{ name: "Build", classification: "unbaselined" }], + ); + // A green aggregate that skipped the failing job must not read as a green baseline for it. + const skippedBaseline = classifyFailedJobs(["Lighthouse budget"], runs[1], [], ["Build"]); + assert.deepEqual(skippedBaseline, [{ name: "Lighthouse budget", classification: "unbaselined" }]); + assert.match(buildTriageBody(skippedBaseline, runs[1]), /did not exercise `Lighthouse budget`/); + assert.match(buildTriageBody(skippedBaseline, runs[1]), /\*\*not baselined\*\*/); + // Omitted executed-name list keeps the previous behaviour rather than inventing a verdict. + assert.deepEqual(classifyFailedJobs(["Lighthouse budget"], runs[1], []), [ + { name: "Lighthouse budget", classification: "needs-investigation" }, + ]); assert.match( buildTriageBody([{ name: "Build", classification: "main-side" }], null), /No completed main CI baseline/, diff --git a/scripts/guard-push.mjs b/scripts/guard-push.mjs index 7d0d5681ca..0beaa0a7c1 100755 --- a/scripts/guard-push.mjs +++ b/scripts/guard-push.mjs @@ -414,13 +414,18 @@ export function defaultRunsFetch(branch, exec = execFileSync) { export function inFlightCiGuard( branches, _ranges = [], - { prViewer = defaultPrView, runFetcher = defaultRunsFetch } = {}, + // `ghAvailable` is injectable for the same reason `prViewer`/`runFetcher` are: without it + // the fail-open below short-circuits before either injected fetcher is consulted, so the + // message-formatting cases could only ever run where the `gh` binary happens to be + // installed. They passed in CI and failed in every container without it, which reads as a + // product regression and is not one. + { prViewer = defaultPrView, runFetcher = defaultRunsFetch, ghAvailable = ghIsAvailable } = {}, ) { void _ranges; if (process.env.SKIP_IN_FLIGHT_CI_GUARD === "1") { return { name: "in-flight-ci", ok: true, skipped: "SKIP_IN_FLIGHT_CI_GUARD=1" }; } - if (!ghIsAvailable()) { + if (!ghAvailable()) { return { name: "in-flight-ci", ok: true, note: "gh not available — in-flight CI check skipped (fail-open)" }; } @@ -1266,6 +1271,9 @@ function selfTest() { const mockBlockedGuard = inFlightCiGuard(["claude/feature"], [], { prViewer: () => ({ state: "OPEN", number: 99 }), runFetcher: () => [activeCiRun], + // Same reason as the injected fetchers: the real probe short-circuits this case on any + // machine without `gh`, so the self-test would silently assert nothing there. + ghAvailable: () => true, }); assert(mockBlockedGuard.ok === false, "inFlightCiGuard blocks on active CI run"); assert( diff --git a/tests/ci-cache-safety.test.ts b/tests/ci-cache-safety.test.ts index 0e84c64680..b56aae2ef6 100644 --- a/tests/ci-cache-safety.test.ts +++ b/tests/ci-cache-safety.test.ts @@ -239,6 +239,18 @@ describe("CI cache safety", () => { expect(concurrency).toContain("cancel-in-progress: ${{ github.event_name != 'push' }}"); expect(concurrency).not.toContain("cancel-in-progress: true"); + // `cancel-in-progress: false` is necessary and NOT sufficient. GitHub keeps at most one + // PENDING run per concurrency group, so a queued main run is cancelled the moment a newer + // merge queues behind the same group — no supersession involved, and the exemption above + // never sees it. Observed 2026-08-20: four consecutive main pushes cancelled while a + // ~70-minute release-browser-matrix held `CI-refs/heads/main`. A per-run group for pushes + // is the part that actually keeps every merged commit verified. + expect(concurrency).toContain("github.event_name == 'push'"); + expect( + concurrency, + "base-branch pushes must key concurrency on github.run_id, or a later merge evicts the pending run", + ).toMatch(/group:.*github\.event_name == 'push'.*github\.run_id/s); + // `on.push.branches` is what makes `event_name == 'push'` mean "base branch" — if a push // trigger is ever widened to feature branches, this exemption silently stops being scoped // and every branch keeps its superseded runs alive. diff --git a/tests/guard-push.test.ts b/tests/guard-push.test.ts index 27a79f32f4..79bbeada87 100644 --- a/tests/guard-push.test.ts +++ b/tests/guard-push.test.ts @@ -464,6 +464,10 @@ describe("in-flight CI push guard (#HSSHRG)", () => { const result = inFlightCiGuard(["claude/my-fix"], [], { prViewer: () => ({ state: "OPEN", number: 77 }), runFetcher: () => runs, + // Without this the guard fails open at the `gh --version` probe and never reaches the + // formatting under test, so the case would assert nothing on any machine that has no + // `gh` on PATH — green in CI, red in a bare container, for no product reason. + ghAvailable: () => true, }); expect(result.ok).toBe(false); expect(result.message).toContain("PR #77 on claude/my-fix has required CI run(s) currently IN-FLIGHT"); @@ -472,6 +476,20 @@ describe("in-flight CI push guard (#HSSHRG)", () => { expect(result.message).toContain("#HSSHRG"); }); + it("inFlightCiGuard fails open when gh is unavailable", () => { + const result = inFlightCiGuard(["claude/my-fix"], [], { + prViewer: () => { + throw new Error("prViewer must not be consulted without gh"); + }, + runFetcher: () => { + throw new Error("runFetcher must not be consulted without gh"); + }, + ghAvailable: () => false, + }); + expect(result.ok).toBe(true); + expect(result.note).toContain("gh not available"); + }); + it("inFlightCiGuard skips when SKIP_IN_FLIGHT_CI_GUARD=1 is set", () => { const previous = process.env.SKIP_IN_FLIGHT_CI_GUARD; process.env.SKIP_IN_FLIGHT_CI_GUARD = "1";