fix(review): judge a CI check by its name's latest run, not by any leftover

packages/cli/src/commands/review/presubmit.test.ts

@@ -158,6 +158,124 @@ describe('classifyCi — a skipped check is not a passing check', () => {
   });
 });
 
+// A name's runs supersede each other: the routing workflows re-dispatch a name
+// several times per commit and cancel the displaced runs, and a flaky job
+// re-run to green leaves its failed attempt behind. Failure used to be judged
+// per RUN, so any one leftover pushed its name into `failedCheckNames` — two
+// real reviews were downgraded from Approve over exactly that (`route` at
+// #7150; route, review-pr, review-config and four more at #7171), each on a
+// commit whose every live check was green on the PR page.
+describe('classifyCi — a superseded run does not outvote the latest verdict', () => {
+  const at = (
+    name: string,
+    conclusion: string | null,
+    completed_at: string | null,
+    status = 'completed',
+  ) => ({ name, status, conclusion, completed_at });
+
+  it('a cancelled run displaced by a later success is not a failure — the #7150/#7171 false alarm', () => {
+    const got = classifyCi(
+      [
+        at('route', 'success', '2026-07-18T15:30:00Z'),
+        at('route', 'cancelled', '2026-07-18T15:10:00Z'),
+        at('route', 'success', '2026-07-18T15:20:00Z'),
+      ],
+      [],
+    );
+    expect(got.failedCheckNames).toEqual([]);
+    expect(got.class).toBe('all_pass');
+  });
+
+  it('a flaky job re-run to green is green — the failed attempt is history', () => {
+    const got = classifyCi(
+      [
+        at(
+          'Test (ubuntu-latest, Node 22.x)',
+          'failure',
+          '2026-07-18T10:00:00Z',
+        ),
+        at(
+          'Test (ubuntu-latest, Node 22.x)',
+          'success',
+          '2026-07-18T11:00:00Z',
+        ),
+      ],
+      [],
+    );
+    expect(got.class).toBe('all_pass');
+  });
+
+  it('a re-run that FAILS after a success is a failure — latest wins both ways', () => {
+    const got = classifyCi(
+      [
+        at('Test', 'success', '2026-07-18T10:00:00Z'),
+        at('Test', 'failure', '2026-07-18T11:00:00Z'),
+      ],
+      [],
+    );
+    expect(got.class).toBe('any_failure');
+    expect(got.failedCheckNames).toEqual(['Test']);
+  });
+
+  it('a name whose ONLY run was cancelled still fails — nothing superseded it', () => {
+    const got = classifyCi(
+      [at('E2E', 'cancelled', '2026-07-18T10:00:00Z')],
+      [],
+    );
+    expect(got.class).toBe('any_failure');
+    expect(got.failedCheckNames).toEqual(['E2E']);
+  });
+
+  it('a later skipped re-dispatch does not erase a real failure — skips are not verdicts', () => {
+    const got = classifyCi(
+      [
+        at('Test', 'failure', '2026-07-18T10:00:00Z'),
+        at('Test', 'skipped', '2026-07-18T11:00:00Z'),
+      ],
+      [],
+    );
+    expect(got.class).toBe('any_failure');
+    expect(got.failedCheckNames).toEqual(['Test']);
+  });
+
+  it('with no timestamps at all, the first-listed run keeps the name — the API lists newest first', () => {
+    const got = classifyCi(
+      [at('route', 'success', null), at('route', 'cancelled', null)],
+      [],
+    );
+    expect(got.class).toBe('all_pass');
+  });
+
+  it('falls back to started_at when completed_at is absent', () => {
+    // The winning run is listed SECOND on purpose: with the fallback dropped
+    // (`completed_at ?? ''`) both stamps collapse to '' and first-seen keeps
+    // the name — so a fixture that lists the success first passes with or
+    // without the fallback and pins nothing. Listed second, the success can
+    // win only through its `started_at`.
+    const got = classifyCi(
+      [
+        {
+          name: 'route',
+          status: 'completed',
+          conclusion: 'cancelled',
+          completed_at: null,
+          started_at: '2026-07-18T15:10:00Z',
+        },
+        {
+          name: 'route',
+          status: 'completed',
+          conclusion: 'success',
+          completed_at: null,
+          started_at: '2026-07-18T15:30:00Z',
+        },
+      ],
+      [],
+    );
+    expect(got.class).toBe('all_pass');
+    expect(got.failedCheckNames).toEqual([]);
+  });
+});
+
 const {
   ghMock,
   ghApiMock,

packages/cli/src/commands/review/presubmit.ts

@@ -48,10 +48,22 @@ interface CheckRun {
   name: string;
   status: string;
   conclusion: string | null;
+  /** ISO timestamps from the API — how re-runs of one name are ordered. */
+  started_at?: string | null;
+  completed_at?: string | null;
   details_url?: string;
   html_url?: string;
 }
 
+/**
+ * When this run's verdict was reached, for ordering re-runs of one name.
+ * ISO-8601 strings compare correctly as strings; a run with no timestamp
+ * sorts earliest, so it can never displace a dated verdict.
+ */
+function verdictStamp(run: CheckRun): string {
+  return run.completed_at ?? run.started_at ?? '';
+}
+
 interface CommitStatus {
   context: string;
   state: string;
@@ -145,13 +157,35 @@ export function classifyCi(checkRuns: CheckRun[], statuses: CommitStatus[]) {
     .filter((n) => !executedNames.has(n))
     .sort();
 
+  // Failure is judged per NAME, like execution above — and by the name's
+  // LATEST verdict, because a name's runs supersede each other: this repo's
+  // routing workflows re-dispatch a name several times per commit and cancel
+  // the displaced runs, and a flaky job re-run to green leaves its failed
+  // attempt behind. Any single failing run used to push its name into
+  // `failedCheckNames`, so a check whose newest run PASSED was reported as
+  // "CI failing" — two real reviews were downgraded from Approve over exactly
+  // that (`route` at #7150, seven routing names at #7171), each on a commit
+  // whose every live check was green. The latest run per name is also what
+  // GitHub's own PR page shows, so this judges the same evidence a human
+  // reviewer sees there. Skipped/neutral/stale runs stay non-verdicts: a
+  // re-dispatch that skipped must not erase a real failure beside it.
+  const latestVerdicts = new Map<string, CheckRun>();
   for (const run of relevantCheckRuns) {
-    if (run.status === 'completed') {
-      if (run.conclusion && FAIL_CONCLUSIONS.has(run.conclusion)) {
-        failedCheckNames.push(run.name);
-      }
-    } else if (PENDING_STATES.has(run.status)) {
-      hasPending = true;
+    if (run.status !== 'completed') {
+      if (PENDING_STATES.has(run.status)) hasPending = true;
+      continue;
+    }
+    if (!run.conclusion || NOT_RUN_CONCLUSIONS.has(run.conclusion)) continue;
+    const prev = latestVerdicts.get(run.name);
+    // Strict `>`: on equal (or absent) stamps the first-seen run keeps the
+    // name, and the API lists newest first.
+    if (!prev || verdictStamp(run) > verdictStamp(prev)) {
+      latestVerdicts.set(run.name, run);
+    }
+  }
+  for (const [name, run] of latestVerdicts) {
+    if (FAIL_CONCLUSIONS.has(run.conclusion as string)) {
+      failedCheckNames.push(name);
     }
   }
   for (const s of statuses) {