fix(pr): stop cancelled CI runs from masking the latest review status (#298)

A codex CI review cancelled when `/review` re-triggers it (concurrency
cancel-in-progress) reports conclusion CANCELLED, which summarizeChecks
treated as a failure. Combined with the fresh run posting under a
different check name, the PR stayed pinned to "failed" and the latest
review status was never surfaced.

Dedupe the rollup to the most recent entry per check name / status
context (latest wins by completion time), drop CANCELLED runs when
summarizing (superseded, not a failing verdict), and map CANCELLED to
skipped in the per-check detail.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: rubenfiszel

backend/src/__tests__/pr.test.ts

@@ -1,6 +1,26 @@
 import { describe, expect, it } from "bun:test";
 import { mapWithConcurrency, startSerializedInterval } from "../lib/async";
-import { parseReviewComments } from "../services/pr-service";
+import {
+  dedupeLatestChecks,
+  mapChecks,
+  parseReviewComments,
+  summarizeChecks,
+} from "../services/pr-service";
+
+// Mirror the GhCheckEntry shape from pr-service (not exported) closely enough
+// for these summarizers, which only read the fields referenced below.
+function checkRun(over: Record<string, unknown> = {}): any {
+  return {
+    __typename: "CheckRun",
+    name: "codex-review",
+    status: "COMPLETED",
+    conclusion: "SUCCESS",
+    detailsUrl: "https://github.com/o/r/actions/runs/1/job/1",
+    startedAt: "2026-07-23T09:00:00Z",
+    completedAt: "2026-07-23T09:05:00Z",
+    ...over,
+  };
+}
 
 describe("parseReviewComments", () => {
   it("parses normal review comments", () => {
@@ -82,6 +102,59 @@ describe("parseReviewComments", () => {
   });
 });
 
+describe("summarizeChecks — cancelled/superseded runs", () => {
+  it("does not report failed when a codex run was cancelled by a re-trigger", () => {
+    // Real-world shape: `/review` cancels the auto codex run (concurrency
+    // cancel-in-progress); the fresh run posts under a different check name.
+    const checks = [
+      checkRun({ name: "codex-review", conclusion: "CANCELLED", completedAt: "2026-07-23T09:32:58Z" }),
+      checkRun({ name: "codex / codex-review", conclusion: "SUCCESS", completedAt: "2026-07-23T09:40:00Z" }),
+    ];
+    expect(summarizeChecks(checks)).toBe("success");
+  });
+
+  it("latest run of the same check name wins over an earlier cancelled one", () => {
+    const checks = [
+      checkRun({ conclusion: "CANCELLED", completedAt: "2026-07-23T09:32:58Z" }),
+      checkRun({ conclusion: "SUCCESS", startedAt: "2026-07-23T09:33:00Z", completedAt: "2026-07-23T09:40:00Z" }),
+    ];
+    expect(summarizeChecks(checks)).toBe("success");
+  });
+
+  it("still reports failed for a genuine failing run", () => {
+    expect(summarizeChecks([checkRun({ conclusion: "FAILURE" })])).toBe("failed");
+  });
+
+  it("reports none when every check is cancelled (no verdict)", () => {
+    expect(summarizeChecks([checkRun({ conclusion: "CANCELLED" })])).toBe("none");
+  });
+
+  it("treats a still-running check as pending despite the zero completedAt sentinel", () => {
+    const checks = [
+      checkRun({ conclusion: "CANCELLED", completedAt: "2026-07-23T09:32:58Z" }),
+      checkRun({ status: "IN_PROGRESS", conclusion: "", startedAt: "2026-07-23T09:35:00Z", completedAt: "0001-01-01T00:00:00Z" }),
+    ];
+    expect(summarizeChecks(checks)).toBe("pending");
+  });
+});
+
+describe("dedupeLatestChecks / mapChecks", () => {
+  it("keeps only the latest entry per check name", () => {
+    const deduped = dedupeLatestChecks([
+      checkRun({ conclusion: "CANCELLED", completedAt: "2026-07-23T09:32:58Z" }),
+      checkRun({ conclusion: "SUCCESS", completedAt: "2026-07-23T09:40:00Z" }),
+    ]);
+    expect(deduped).toHaveLength(1);
+    expect((deduped[0] as any).conclusion).toBe("SUCCESS");
+  });
+
+  it("maps a cancelled run to skipped rather than failed", () => {
+    const mapped = mapChecks([checkRun({ name: "solo", conclusion: "CANCELLED" })]);
+    expect(mapped).toHaveLength(1);
+    expect(mapped[0].status).toBe("skipped");
+  });
+});
+
 describe("mapWithConcurrency", () => {
   it("maps all items with results in order", async () => {
     const items = [1, 2, 3, 4, 5];

backend/src/services/pr-service.ts

@@ -49,6 +49,8 @@ interface GhCheckRunEntry {
   status: GhCheckStatus;
   name: string;
   detailsUrl: string | null;
+  startedAt?: string | null;
+  completedAt?: string | null;
 }
 
 // StatusContext entries from external CI (e.g. Vercel)
@@ -57,6 +59,7 @@ interface GhStatusContextEntry {
   context: string;
   state: "SUCCESS" | "FAILURE" | "PENDING" | "ERROR" | "EXPECTED";
   targetUrl: string | null;
+  createdAt?: string | null;
 }
 
 type GhCheckEntry = GhCheckRunEntry | GhStatusContextEntry;
@@ -88,18 +91,68 @@ const etagCache = new Map<string, { etag: string; comments: PrComment[] }>();
 
 // ── Pure helper functions (exported for unit testing) ─────────────────────────
 
+/** Group key for an entry: check-run name, or external status context. */
+function checkEntryKey(c: GhCheckEntry): string {
+  return c.__typename === "StatusContext" ? `status:${c.context}` : `check:${c.name}`;
+}
+
+/** Parse an ISO timestamp to epoch ms, 0 when absent/invalid. */
+function toEpoch(ts: string | null | undefined): number {
+  const t = ts ? new Date(ts).getTime() : NaN;
+  return Number.isNaN(t) ? 0 : t;
+}
+
+/**
+ * Recency of an entry (epoch ms) for latest-wins dedupe. For a check run, use
+ * the later of startedAt/completedAt: GitHub reports completedAt as a zero
+ * sentinel ("0001-01-01T00:00:00Z") while still running, so a live run would
+ * otherwise sort as ancient and lose to an older completed run.
+ */
+function checkEntryTime(c: GhCheckEntry): number {
+  if (c.__typename === "StatusContext") return toEpoch(c.createdAt);
+  return Math.max(toEpoch(c.startedAt), toEpoch(c.completedAt));
+}
+
+/** A CANCELLED check-run is a superseded/aborted run, not a failing verdict. */
+function isCancelled(c: GhCheckEntry): boolean {
+  return c.__typename === "CheckRun" && c.conclusion === "CANCELLED";
+}
+
+/**
+ * Collapse the rollup to the most recent entry per check name / status context.
+ * Re-triggering a workflow (e.g. `/review` re-running codex CI) leaves the prior
+ * run in the rollup under the same name; keeping only the latest lets the fresh
+ * result win instead of a stale run masking it. Latest wins by completion time,
+ * falling back to array order (GitHub returns oldest-first) on ties.
+ */
+export function dedupeLatestChecks(checks: GhCheckEntry[]): GhCheckEntry[] {
+  const latest = new Map<string, GhCheckEntry>();
+  for (const c of checks) {
+    const key = checkEntryKey(c);
+    const prev = latest.get(key);
+    if (!prev || checkEntryTime(c) >= checkEntryTime(prev)) latest.set(key, c);
+  }
+  return [...latest.values()];
+}
+
 /** Summarize CI check status from a statusCheckRollup array. */
 export function summarizeChecks(
   checks: GhCheckEntry[] | null,
 ): PrEntry["ciStatus"] {
   if (!checks || checks.length === 0) return "none";
-  const allDone = checks.every((c) =>
+  // Drop CANCELLED runs: a codex CI review cancelled when `/review` re-triggers
+  // it (concurrency cancel-in-progress) reports CANCELLED, which must not mask
+  // the latest run — otherwise the PR stays "failed" forever. Also dedupe so a
+  // same-name re-run's fresh result wins over the superseded one.
+  const relevant = dedupeLatestChecks(checks).filter((c) => !isCancelled(c));
+  if (relevant.length === 0) return "none";
+  const allDone = relevant.every((c) =>
     c.__typename === "StatusContext"
       ? c.state !== "PENDING" && c.state !== "EXPECTED"
       : c.status === "COMPLETED",
   );
   if (!allDone) return "pending";
-  const allPass = checks.every((c) => {
+  const allPass = relevant.every((c) => {
     if (c.__typename === "StatusContext") return c.state === "SUCCESS";
     return c.conclusion === "SUCCESS" || c.conclusion === "NEUTRAL" || c.conclusion === "SKIPPED";
   });
@@ -124,14 +177,15 @@ export function deriveCheckStatus(check: GhCheckEntry): CiCheck["status"] {
   if (check.status !== "COMPLETED") return "pending";
   const c = check.conclusion;
   if (c === "SUCCESS" || c === "NEUTRAL") return "success";
-  if (c === "SKIPPED") return "skipped";
+  // CANCELLED = superseded (e.g. codex CI re-triggered by `/review`); not a failure.
+  if (c === "SKIPPED" || c === "CANCELLED") return "skipped";
   return "failed";
 }
 
 /** Map raw GH check entries to typed CiCheck array. */
 export function mapChecks(checks: GhCheckEntry[] | null): CiCheck[] {
   if (!checks || checks.length === 0) return [];
-  return checks.map((c) => {
+  return dedupeLatestChecks(checks).map((c) => {
     const name = c.__typename === "StatusContext" ? c.context : c.name;
     const url = c.__typename === "StatusContext" ? c.targetUrl : c.detailsUrl;
     return {