Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions .github/workflows/ci-triage.yml
Original file line numberDiff line numberDiff line change
Expand Up@@ -50,6 +50,7 @@ jobs:
const {
buildTriageBody,
classifyFailedJobs,
executedJobNames,
failedJobNames,
selectLatestDefaultBranchRun,
} = await import(moduleUrl);
Expand DownExpand Up@@ -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,
Expand All@@ -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 = "<!-- ci-triage -->";

Expand Down
14 changes: 13 additions & 1 deletion .github/workflows/ci.yml
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand Down
105 changes: 94 additions & 11 deletions scripts/ci-triage.mjs
Original file line numberDiff line numberDiff line change
Expand Up@@ -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(
Expand All@@ -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 = "<!-- ci-triage -->";
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,
Expand DownExpand Up@@ -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",
},
{
Expand All@@ -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/,
Expand Down
12 changes: 10 additions & 2 deletions scripts/guard-push.mjs
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)" };
}

Expand DownExpand Up@@ -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(
Expand Down
12 changes: 12 additions & 0 deletions tests/ci-cache-safety.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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.
Expand Down
18 changes: 18 additions & 0 deletions tests/guard-push.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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");
Expand All@@ -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";
Expand Down
Loading