fix(ci): name the failing shard in the merge-queue ejection comment (BLO-28886)

.github/scripts/report-merge-queue-ejection.mjs

@@ -44,6 +44,100 @@ export function shouldReportCancelledRun({ merged, isInMergeQueue }) {
   return !merged && !isInMergeQueue;
 }
 
+// BLO-28886 asks the ejection signal to NAME the failing shard. Without it the
+// comment says "inspect the merge-group jobs", i.e. it re-hands the lookup back
+// to the reader -- and the whole point of this reporter is that a queue-branch
+// failure is invisible from the PR, so that lookup is exactly the expensive bit.
+//
+// No name-filtering of aggregator jobs: a name allowlist rots on the next
+// workflow rename. Aggregators are excluded STRUCTURALLY instead -- see
+// causalJobs(). An earlier revision of this comment argued "one extra job name
+// costs a reader nothing"; measured, that was wrong. On the two `policy`
+// timeout ejections of 2026-09-23/25 the extra name was the ONLY name, and it
+// was the wrong one.
+//
+// The filter is on CONCLUSION, not name. A `cancelled` job is only signal when
+// nothing else failed: fail-fast cancels the siblings of a job that genuinely
+// failed, so on run 35993984182 (ejected #1976) `Build` is cancelled collateral
+// of `General tests (workspaces-a)`. Naming it sends the reader to a log that
+// contains nothing but a cancellation -- the same misdirection runOutcomeText
+// exists to prevent, one layer down. When nothing failed, a cancelled job IS the
+// cause (a `timeout-minutes` kill surfaces that way) and must still be named.
+//
+// Both rules below need the same input -- the non-success jobs that could
+// actually be a CAUSE -- so they share causalJobs() rather than each deriving
+// it. That sharing is the point: the two used to disagree, and a reader only
+// ever sees the sentence they compose.
+const isNonSuccess = (job) => job?.conclusion === "failure" || job?.conclusion === "cancelled";
+
+/**
+ * Non-success jobs minus the aggregator lanes that merely report upstream death.
+ *
+ * `verify` needs: every other lane, so it goes red whenever anything it waits
+ * on dies -- it is a MESSENGER, never a cause. Naming it is the same
+ * misdirection as naming fail-fast collateral, and it is the one the
+ * conclusion filter above cannot catch, because `verify` is a genuine
+ * `failure` rather than a `cancelled`. Measured on the two `policy`
+ * timeout ejections in the 2026-09-23/25 window (runs 35948766367 and
+ * 36015721472): the comment read "The merge-group run failed. Failing job:
+ * `verify`." -- the reader is sent to a log whose entire content is "upstream
+ * lane(s) did not run", and `policy`, killed at its 600s cap, is never named.
+ *
+ * Detected structurally, not by name: an allowlist of aggregator names rots on
+ * the next workflow rename. A job that starts only after EVERY other
+ * non-success job has finished ran last by construction, which is what being
+ * downstream of all of them means. Deliberately the narrowest form of that
+ * test -- "after SOME other job" would drop a genuine second failure that
+ * merely started late.
+ */
+export function causalJobs(jobs) {
+  const list = (Array.isArray(jobs) ? jobs : []).filter(isNonSuccess);
+  const at = (value) => Date.parse(value ?? "");
+  const causal = list.filter((job) => {
+    const started = at(job.started_at);
+    const others = list.filter((other) => other !== job);
+    if (!others.length || Number.isNaN(started)) return true;
+    return !others.every((other) => {
+      const done = at(other.completed_at);
+      return !Number.isNaN(done) && started >= done;
+    });
+  });
+  // A single non-success job is vacuously "last"; never report nothing when
+  // something did fail.
+  return causal.length ? causal : list;
+}
+
+export function failingJobSummary(jobs) {
+  const list = causalJobs(jobs);
+  const anyFailure = list.some((job) => job?.conclusion === "failure");
+  const failed = list
+    .filter((job) => job?.conclusion === "failure" || (!anyFailure && job?.conclusion === "cancelled"))
+    .map((job) => job.name)
+    .filter((name) => typeof name === "string" && name.length > 0);
+  if (failed.length === 0) return "";
+  return ` Failing ${failed.length === 1 ? "job" : "jobs"}: ${failed.map((name) => `\`${name}\``).join(", ")}.`;
+}
+
+// A run whose conclusion is `cancelled` is NOT necessarily a timeout. GitHub
+// marks the whole run `cancelled` when ANY job is cancelled, and fail-fast
+// cancels the siblings of a job that genuinely failed -- so the commonest
+// `cancelled` run is a real test failure wearing a timeout's clothes. Measured
+// on run 35993984182 (ejected #1976): run `cancelled`, but `General tests
+// (workspaces-a)` and `verify` both `failure`, and the actual cause was an
+// after-teardown `ReferenceError: window is not defined` with 3075/3075 tests
+// passing. Reading the run-level conclusion alone sends the reader to look for
+// an infra timeout that is not there.
+//
+// Reads the same causalJobs() set as failingJobSummary: `verify`'s own
+// `failure` used to flip this to "failed" on a run whose real cause was
+// `policy` hitting its cap, so the sentence asserted a failure and then named
+// only the messenger.
+export function runOutcomeText(conclusion, jobs) {
+  const failed =
+    conclusion === "failure" || causalJobs(jobs).some((job) => job?.conclusion === "failure");
+  return failed ? "failed" : "was cancelled (a job timeout surfaces this way)";
+}
+
 async function githubRequest(path, options = {}) {
   const response = await fetch(`https://api.github.com${path}`, {
     ...options,
@@ -117,8 +211,21 @@ export async function reportMergeQueueFailure({ repository, headBranch, conclusi
     return { reported: false, reason: "already-reported", number };
   }
 
-  const outcome = conclusion === "failure" ? "failed" : "was cancelled (a job timeout surfaces this way)";
-  const body = `${marker}\nMerge-queue ejection detected for PR #${number}. The merge-group run ${outcome} and GitHub may have removed the PR from the queue and dropped auto-merge. Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR.\n\nRun: ${runUrl}`;
+  // Best-effort: a failure here must NOT lose the ejection report. Losing the
+  // shard name degrades the comment; losing the comment leaves a stuck PR
+  // silent, which is the failure this whole script exists to prevent.
+  // One unpaginated page (default `filter=latest`, so a re-run's older attempts
+  // are excluded). Ceiling: a workflow past 100 jobs truncates -- 17 today.
+  let jobs;
+  try {
+    ({ jobs } = await githubRequest(
+      `/repos/${owner}/${repo}/actions/runs/${runId}/jobs?per_page=100`,
+    ));
+  } catch (error) {
+    console.warn(`Could not read jobs for run ${runId}: ${error.message}`);
+  }
+  const summary = failingJobSummary(jobs);
+  const body = `${marker}\nMerge-queue ejection detected for PR #${number}. The merge-group run ${runOutcomeText(conclusion, jobs)} and GitHub may have removed the PR from the queue and dropped auto-merge.${summary} Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR.\n\nRun: ${runUrl}`;
   await githubRequest(`/repos/${owner}/${repo}/issues/${number}/comments`, {
     method: "POST",
     headers: { "content-type": "application/json" },

.github/scripts/tests/report-merge-queue-ejection.test.mjs

@@ -4,6 +4,9 @@ import {
   mergeQueuePullRequestNumber,
   shouldReportMergeQueueFailure,
   shouldReportCancelledRun,
+  failingJobSummary,
+  runOutcomeText,
+  causalJobs,
 } from "../report-merge-queue-ejection.mjs";
 
 test("extracts PR numbers from merge-group synthetic refs", () => {
@@ -37,3 +40,149 @@ test("a cancelled run is reported only when the PR is neither merged nor still q
   // Still queued for a re-build: not ejected.
   assert.equal(shouldReportCancelledRun({ merged: false, isInMergeQueue: true }), false);
 });
+
+test("the ejection comment names the failing shards", () => {
+  // Shape taken from the real run that ejected #1962 (35871782486). No
+  // timestamps here on purpose: causalJobs() can only prove a lane is
+  // downstream from them, so without them it keeps everything and this
+  // degrades to naming the aggregator too. Fail-safe direction -- one extra
+  // name, never a missing one. The timestamped cases below are the real shape.
+  assert.equal(
+    failingJobSummary([
+      { name: "General tests (workspaces-a)", conclusion: "failure" },
+      { name: "General tests (server 1/4)", conclusion: "success" },
+      { name: "verify", conclusion: "failure" },
+    ]),
+    " Failing jobs: `General tests (workspaces-a)`, `verify`.",
+  );
+  // A job killed by `timeout-minutes` surfaces as cancelled, and that is the
+  // class pr.yml deliberately trades a fast red for -- so it must be named too.
+  assert.equal(
+    failingJobSummary([{ name: "General tests (server 4/4)", conclusion: "cancelled" }]),
+    " Failing job: `General tests (server 4/4)`.",
+  );
+  // Empty, not a dangling "Failing jobs:", when the jobs read gave nothing
+  // usable -- the comment must still read as a sentence. Covers the
+  // best-effort catch path, which passes no jobs at all.
+  assert.equal(failingJobSummary([{ name: "verify", conclusion: "success" }]), "");
+  assert.equal(failingJobSummary([]), "");
+  assert.equal(failingJobSummary(undefined), "");
+});
+
+test("a `cancelled` run carrying a failed job is described as failed, not as a timeout", () => {
+  // Shape taken from run 35993984182, which ejected #1976: run-level conclusion
+  // `cancelled` (fail-fast cancelled `Build`), but the cause was a genuine
+  // `failure` in a shard. Reporting that as "a job timeout surfaces this way"
+  // points the reader at infra instead of at the shard.
+  assert.equal(
+    runOutcomeText("cancelled", [
+      { name: "Build", conclusion: "cancelled" },
+      { name: "General tests (workspaces-a)", conclusion: "failure" },
+      { name: "verify", conclusion: "failure" },
+    ]),
+    "failed",
+  );
+  // A genuine timeout: every non-success job is cancelled, nothing failed.
+  assert.equal(
+    runOutcomeText("cancelled", [
+      { name: "General tests (server 4/4)", conclusion: "cancelled" },
+      { name: "verify", conclusion: "success" },
+    ]),
+    "was cancelled (a job timeout surfaces this way)",
+  );
+  // Degrades to the run-level conclusion when the best-effort jobs read gave
+  // nothing -- i.e. exactly the pre-BLO-28886 behaviour, never worse.
+  assert.equal(runOutcomeText("cancelled", undefined), "was cancelled (a job timeout surfaces this way)");
+  assert.equal(runOutcomeText("failure", undefined), "failed");
+});
+
+test("the rendered sentence is coherent on a fail-fast run: no cancelled collateral is named", () => {
+  // The two functions above are each correct in isolation while the sentence
+  // they compose is wrong -- that split is what hid the `Build` case. So assert
+  // on BOTH over ONE real shape: run 35993984182, verbatim from the API.
+  // Untimestamped, so `verify` survives; the timestamped form of this same run
+  // is asserted below, where it correctly drops to the shard alone.
+  const jobs = [
+    { name: "Build", conclusion: "cancelled" },
+    { name: "General tests (workspaces-a)", conclusion: "failure" },
+    { name: "General tests (server 1/4)", conclusion: "success" },
+    { name: "verify", conclusion: "failure" },
+  ];
+  assert.equal(
+    `The merge-group run ${runOutcomeText("cancelled", jobs)}.${failingJobSummary(jobs)}`,
+    "The merge-group run failed. Failing jobs: `General tests (workspaces-a)`, `verify`.",
+  );
+  // `Build` was cancelled BECAUSE workspaces-a failed; its log shows only a
+  // cancellation. Naming it is the misdirection, not extra detail.
+  assert.ok(!failingJobSummary(jobs).includes("Build"));
+});
+
+test("a `policy` timeout is named as the cause, not the `verify` lane that merely reports it", () => {
+  // Run 35948766367, verbatim from the API: an ejection whose cause is `policy`
+  // killed at its 600s cap. `verify` needs: it, so `verify` goes red 14s later
+  // and is a genuine `failure` -- which the conclusion filter cannot catch.
+  // Before causalJobs() this rendered "The merge-group run failed. Failing job:
+  // `verify`.", sending the reader to a log reading only "upstream lane(s) did
+  // not run". 2 of the 7 merge-group ejections in the 2026-09-23/25 window.
+  const jobs = [
+    { name: "policy", conclusion: "cancelled", started_at: "2026-09-24T02:57:04Z", completed_at: "2026-09-24T03:08:22Z" },
+    { name: "verify", conclusion: "failure", started_at: "2026-09-24T03:08:36Z", completed_at: "2026-09-24T03:08:47Z" },
+  ];
+  assert.equal(
+    `The merge-group run ${runOutcomeText("cancelled", jobs)}.${failingJobSummary(jobs)}`,
+    "The merge-group run was cancelled (a job timeout surfaces this way). Failing job: `policy`.",
+  );
+});
+
+test("a late-starting second failure is still named: only a job after ALL others is dropped", () => {
+  // The narrowness guard. `Build` starts before `workspaces-a` finishes, so it
+  // is not downstream-of-everything and must survive causalJobs() -- it is then
+  // dropped as fail-fast collateral by the conclusion filter, which is a
+  // different rule. Widening causalJobs to "after SOME other job" would drop a
+  // genuine independent failure that merely started late.
+  const jobs = [
+    { name: "Build", conclusion: "cancelled", started_at: "2026-09-24T11:46:54Z", completed_at: "2026-09-24T12:12:33Z" },
+    { name: "General tests (workspaces-a)", conclusion: "failure", started_at: "2026-09-24T11:45:42Z", completed_at: "2026-09-24T12:05:41Z" },
+    { name: "verify", conclusion: "failure", started_at: "2026-09-24T13:36:47Z", completed_at: "2026-09-24T13:37:41Z" },
+  ];
+  assert.deepEqual(causalJobs(jobs).map((job) => job.name), ["Build", "General tests (workspaces-a)"]);
+  assert.equal(
+    `The merge-group run ${runOutcomeText("cancelled", jobs)}.${failingJobSummary(jobs)}`,
+    "The merge-group run failed. Failing job: `General tests (workspaces-a)`.",
+  );
+});
+
+test("a lone failing job is never dropped for being vacuously last", () => {
+  const jobs = [{ name: "e2e", conclusion: "failure", started_at: "2026-09-24T11:45:42Z", completed_at: "2026-09-24T12:05:41Z" }];
+  assert.equal(failingJobSummary(jobs), " Failing job: `e2e`.");
+  // Jobs with no timestamps at all (never dispatched) must not vanish either.
+  assert.equal(failingJobSummary([{ name: "policy", conclusion: "cancelled" }]), " Failing job: `policy`.");
+});
+
+test("a job that started after SOME but not ALL others is kept (guards the narrowness of causalJobs)", () => {
+  // The `every` in causalJobs has to be `every`, not `some`. `b` starts after
+  // `a` finished but while `c` is still running, so it is NOT last and is a
+  // genuine independent failure. Under a `some` test it would silently vanish
+  // from the comment. The fail-fast fixture above cannot catch this -- no job
+  // in it starts after any other one completes -- which is why the widened
+  // form passed the suite before this test existed.
+  const jobs = [
+    { name: "a", conclusion: "failure", started_at: "2026-09-24T10:00:00Z", completed_at: "2026-09-24T10:10:00Z" },
+    { name: "b", conclusion: "failure", started_at: "2026-09-24T10:15:00Z", completed_at: "2026-09-24T10:40:00Z" },
+    { name: "c", conclusion: "failure", started_at: "2026-09-24T10:05:00Z", completed_at: "2026-09-24T10:50:00Z" },
+  ];
+  assert.deepEqual(causalJobs(jobs).map((job) => job.name), ["a", "b", "c"]);
+});
+
+test("jobs cancelled at the same instant are all reported, not all dropped", () => {
+  // Reachability of the `causal.length ? causal : list` fallback. Job timestamps
+  // are second-resolution, so two lanes cancelled together when the run died
+  // carry identical values -- each then reads as "started after the other
+  // finished" and both get dropped, leaving the comment naming nothing at all.
+  // Observed shape: needs:-gated lanes cancelled 0s after creation.
+  const jobs = [
+    { name: "Build", conclusion: "cancelled", started_at: "2026-09-24T10:00:00Z", completed_at: "2026-09-24T10:00:00Z" },
+    { name: "e2e", conclusion: "cancelled", started_at: "2026-09-24T10:00:00Z", completed_at: "2026-09-24T10:00:00Z" },
+  ];
+  assert.equal(failingJobSummary(jobs), " Failing jobs: `Build`, `e2e`.");
+});