fix(prs): address review — canonical rollup outranks raw check rows

CodeRabbit review on #1004. Eleven findings; ten applied, one refuted.

The substantive one: a `not_run` rollup can coexist with pending or
failing THIRD-PARTY rows, and several surfaces evaluated those raw counts
first. A commit nothing verified would report "2 pending checks" or a
failing count instead of the honest answer — the same producer-blind
claim in a different tense. The canonical verdict now takes precedence
in the merge checklist, the merge blockers, and the ade code right pane.

Also:
* A stale commit whose CI was entirely skipped, with a required context
  that never reported, stayed `pending` forever: the guard admitted any
  producer rather than an actual pass.
* `fetchCombinedStatus` sat unwrapped in the same Promise.all as the
  guarded check-runs fetch, so a 403 there still aborted the whole
  refresh instead of preserving the last-known rollup.
* A supplied canonical `checksStatus` is authoritative in the TUI
  formatter; the row fallback speaks only when no verdict was sent.
* iOS bootstrap SQL declares the two ADE-135 columns, so a fresh install
  has them before cr-sqlite delivers a changeset carrying them.
* Fixture and naming corrections: browserMock rows carry real app slugs
  (an absent slug is CI-eligible by design, which silently defeated the
  fixture), the denylist test is named for the rule it asserts, the lane
  summary test is named for the path it actually exercises, and the iOS
  #988 preview clone no longer points at #559's URL.

Refuted: the mock row store's positional read was reported as broken by
the new upsert params. It targets a different UPDATE (the projection
write, unchanged here) and was already inert for the other one. Its
matcher is now specific so the two cannot be confused again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: arul28

apps/ade-cli/src/tuiClient/components/RightPane.tsx

@@ -235,12 +235,13 @@ export function computeLaneChatCounts(
 type LaneDetailsPr = NonNullable<Extract<RightPaneContent, { kind: "lane-details" }>["pr"]>;
 
 function laneDetailsPrChecksLineColor(pr: LaneDetailsPr): string {
+  // ADE-135: the canonical verdict outranks the row counts. A `not_run` rollup
+  // coexists with pending/failing THIRD-PARTY rows, so checking those first
+  // would colour an unverified commit as merely running or red-for-the-wrong-
+  // reason. An unverified commit is the finding, so it takes attention.
+  if (pr.checksStatus === "not_run") return theme.color.attention;
   if (pr.checksPending > 0) return theme.color.running;
   if (pr.checksFailed > 0) return theme.color.error;
-  // ADE-135: an unverified commit is not a quiet neutral state — it is the
-  // finding — so it takes the attention colour rather than the muted one a
-  // clean pass gets.
-  if (pr.checksStatus === "not_run") return theme.color.attention;
   return theme.color.t3;
 }
 
@@ -251,13 +252,6 @@ function laneDetailsPrChipStatus(state: LaneDetailsPr["state"]): "info" | "done"
 }
 
 function formatPrActivity(pr: LaneDetailsPr): string {
-  if (pr.checksPending > 0) {
-    const done = pr.checksTotal - pr.checksPending;
-    return `CI running · ${done}/${pr.checksTotal} done`;
-  }
-  if (pr.checksFailed > 0) {
-    return `${pr.checksFailed} check${pr.checksFailed === 1 ? "" : "s"} failing`;
-  }
   // ADE-135: this said "checks passing" for any non-empty row list, which is
   // exactly the ticket — on PR #988 three bot rows made `checksTotal` 3 while
   // `checksPassed` stayed 0, and the line claimed a pass nothing had earned.
@@ -267,6 +261,14 @@ function formatPrActivity(pr: LaneDetailsPr): string {
       ? `CI not run · ${pr.checksTotal} check${pr.checksTotal === 1 ? "" : "s"}, none from CI`
       : "CI not run";
   }
+
+  if (pr.checksPending > 0) {
+    const done = pr.checksTotal - pr.checksPending;
+    return `CI running · ${done}/${pr.checksTotal} done`;
+  }
+  if (pr.checksFailed > 0) {
+    return `${pr.checksFailed} check${pr.checksFailed === 1 ? "" : "s"} failing`;
+  }
   if (pr.checksPassed > 0) {
     return "checks passing";
   }

apps/ade-cli/src/tuiClient/rightPaneFormatters.ts

@@ -377,7 +377,11 @@ export function formatPrChecks(value: unknown): string {
   const ok = rowRollup.counts.passing;
   const fail = rowRollup.counts.failing;
   const wait = rowRollup.counts.pending;
-  const notRun = rollup === "not_run" || rowRollup.status === "not_run";
+  // A supplied canonical `checksStatus` is authoritative: only the host knows
+  // about required contexts, the merge box, and the grace window. The row
+  // fallback speaks only when the payload carried no verdict at all — letting
+  // it override a supplied `passing` would contradict the contract above.
+  const notRun = rollup ? rollup === "not_run" : rowRollup.status === "not_run";
   const summary = notRun
     ? `CI: not run${reason ? ` — ${reason}` : ""}`
     : [ok ? `${ok} passing` : null, fail ? `${fail} failing` : null, wait ? `${wait} pending` : null]

apps/desktop/src/main/services/prs/prService.test.ts

@@ -432,7 +432,12 @@ function installPullRequestRowStore(db: ReturnType<typeof makeMockDb>, initialRo
 
   db.run.mockImplementation((sql: string, params: unknown[] = []) => {
     const text = String(sql);
-    if (text.includes("update pull_requests")) {
+    // Matches ONLY the projection update in `applyProjectionToLinkedPrRows`
+    // (its params end `… head_sha, id, project_id`). The substring used to be
+    // the bare "update pull_requests", which also matched `upsertRow`'s much
+    // longer UPDATE and then read two arbitrary columns as the id/project id —
+    // silently finding no row instead of failing loudly.
+    if (text.includes("update pull_requests") && text.includes("head_sha = coalesce(?, head_sha)")) {
       const prId = params[12];
       const projectIdParam = params[13];
       const row = rows.find((entry) => entry.id === prId && entry.project_id === projectIdParam);
@@ -843,11 +848,14 @@ describe("prService.getForLane", () => {
     });
   });
 
-  it("does not report a lane PR as fully passed when only bots reported", async () => {
-    // ADE-135: `checksPassed` counted any `success` row with no producer
-    // awareness, so a lane whose PR had only CodeRabbit/Vercel checks reported
-    // N/N — and every consumer inferring a pass from `passed === total`
-    // (the ade code drawer, the lane rail) painted it green.
+  it("carries the canonical checks rollup onto every lane PR summary", async () => {
+    // ADE-135: consumers (the ade code drawer, the lane rail) used to infer a
+    // pass from `checksPassed === checksTotal`, which is producer-blind. They
+    // read `checksStatus` now, so it has to survive the trip through
+    // `listPrsByLane`. This fixture seeds no snapshot rows, so it pins the
+    // no-checks path where the summary echoes the row's stored verdict; the
+    // producer-aware COUNTING it delegates to is covered by the
+    // `rollupPrChecks` suite in shared/prChecksRollup.test.ts.
     const lane = makeFakeLane({ id: "lane-bots", branchRef: "refs/heads/bots-feature" });
     const service = buildGetForLaneService(lane, [
       makePrRow({

apps/desktop/src/main/services/prs/prService.ts

@@ -4779,7 +4779,8 @@ export function createPrService({
      */
     headActivityAt: string | null;
     /**
-     * `bestEffort` turns a 403/rate-limit on /check-runs into `[]`, which is
+     * Set when EITHER checks fetch failed. `bestEffort` turns a 403/rate-limit
+     * on /check-runs or /commits/{sha}/status into an empty result, which is
      * byte-identical to "this commit has no checks". Recomputing from that
      * would flip a green PR to `not_run` and persist it to every surface,
      * so a failed fetch keeps whatever we last knew.
@@ -4846,7 +4847,7 @@ export function createPrService({
     let checkRunsFetchFailed = false;
     const [combinedStatus, checkRuns, reviews, compare] = shouldFetchLiveStatus
       ? await Promise.all([
-          headSha ? fetchCombinedStatus(repo, headSha) : Promise.resolve({ state: "", statuses: [] }),
+          headSha ? bestEffort("refreshOne.fetchCombinedStatus", fetchCombinedStatus(repo, headSha), { state: "", statuses: [] }, () => { checkRunsFetchFailed = true; }) : Promise.resolve({ state: "", statuses: [] }),
           headSha ? bestEffort("refreshOne.fetchCheckRuns", fetchCheckRuns(repo, headSha), [] as any[], () => { checkRunsFetchFailed = true; }) : Promise.resolve([]),
           bestEffort("refreshOne.fetchReviews", fetchReviews(repo, Number(row.github_pr_number)), []),
           baseSha && headSha ? bestEffort("refreshOne.fetchCompare", fetchCompare(repo, baseSha, headSha), { behindBy: null as number | null }) : Promise.resolve({ behindBy: null as number | null })
@@ -5173,7 +5174,7 @@ export function createPrService({
 
     let checkRunsFetchFailed = false;
     const [combinedStatus, checkRuns, reviews, compare, mergeState] = await Promise.all([
-      restHeadSha ? fetchCombinedStatus(repo, restHeadSha) : Promise.resolve({ state: "", statuses: [] }),
+      restHeadSha ? bestEffort("computeStatus.fetchCombinedStatus", fetchCombinedStatus(repo, restHeadSha), { state: "", statuses: [] }, () => { checkRunsFetchFailed = true; }) : Promise.resolve({ state: "", statuses: [] }),
       restHeadSha ? bestEffort("computeStatus.fetchCheckRuns", fetchCheckRuns(repo, restHeadSha), [] as any[], () => { checkRunsFetchFailed = true; }) : Promise.resolve([]),
       bestEffort("computeStatus.fetchReviews", fetchReviews(repo, prNumber), []),
       baseSha && restHeadSha ? bestEffort("computeStatus.fetchCompare", fetchCompare(repo, baseSha, restHeadSha), { behindBy: null as number | null }) : Promise.resolve({ behindBy: null as number | null }),

apps/desktop/src/renderer/browserMock.ts

@@ -1633,13 +1633,18 @@ const MOCK_CHECKS_BY_PR: Record<string, any[]> = {
   // Three green rows, zero CI: a review bot, a preview deploy, and a comment
   // bot. The rollup calls this "not_run" — the rows are real, the pass is not.
   "pr-5": [
+    // Slugs are load-bearing: an ABSENT `appSlug` is treated as CI-eligible
+    // (legacy rows carry none), so without them a producer-aware consumer like
+    // `groupCheckItems` would file all three under CI and the fixture would
+    // stop demonstrating the bug it exists to demonstrate.
     {
       name: "CodeRabbit",
       status: "completed",
       conclusion: "success",
       detailsUrl: "#",
       startedAt: now,
       completedAt: now,
+      appSlug: "coderabbitai",
     },
     {
       name: "Vercel — Preview",
@@ -1648,6 +1653,7 @@ const MOCK_CHECKS_BY_PR: Record<string, any[]> = {
       detailsUrl: "#",
       startedAt: now,
       completedAt: now,
+      appSlug: "vercel",
     },
     {
       name: "changeset-bot",
@@ -1656,6 +1662,7 @@ const MOCK_CHECKS_BY_PR: Record<string, any[]> = {
       detailsUrl: "#",
       startedAt: now,
       completedAt: now,
+      appSlug: "changeset-bot",
     },
   ],
 };

apps/desktop/src/renderer/components/prs/shared/prMergeRailUtils.test.ts

@@ -261,6 +261,32 @@ describe("buildDefaultCommitMessage", () => {
     expect(result.body).toBe("");
   });
 
+  it("reports no-CI rather than a producer-blind pending or failing count", () => {
+    // A not_run rollup coexists with third-party rows in any state. Reporting
+    // "1 pending check" there is the same blind claim in a different tense.
+    for (const conclusion of ["failure", null] as const) {
+      const items = buildMergeChecklist({
+        pr: makePr({ checksStatus: "not_run" }),
+        status: null,
+        checks: [
+          {
+            name: "Vercel",
+            status: conclusion === null ? "in_progress" : "completed",
+            conclusion,
+            detailsUrl: null,
+            startedAt: null,
+            completedAt: null,
+            appSlug: "vercel",
+          },
+        ],
+        reviews: [],
+      });
+      const row = items.find((item) => item.id === "checks");
+      expect(row?.label, String(conclusion)).toBe("No CI has run on this commit");
+      expect(row?.state, String(conclusion)).toBe("neutral");
+    }
+  });
+
   it("does not claim all checks passed when nothing verified the commit", () => {
     // ADE-135: summarizeChecks is producer-blind, so three third-party
     // successes reported passing: 3 and this row said "All 3 checks passed" —

apps/desktop/src/renderer/components/prs/shared/prMergeRailUtils.ts

@@ -89,12 +89,15 @@ export function deriveMergeBlockers(args: {
   const checkSummary = summarizeChecks(checks);
   const blockersRollup = status?.checksStatus ?? pr.checksStatus;
   if (blockersRollup === "not_run") {
+    // The canonical verdict replaces the row counts rather than sitting beside
+    // them: when nothing verified the commit, any failing/pending rows are
+    // third-party, and reporting "2 pending required checks" would be the same
+    // producer-blind claim in a different tense.
     blockers.push({
       id: "no-ci",
       label: "No CI has run on this commit.",
     });
-  }
-  if (checkSummary.failing > 0) {
+  } else if (checkSummary.failing > 0) {
     blockers.push({
       id: "failing-checks",
       label: `${checkSummary.failing} required check${checkSummary.failing === 1 ? "" : "s"} ${checkSummary.failing === 1 ? "is" : "are"} failing.`,
@@ -231,7 +234,17 @@ export function buildMergeChecklist(args: {
   // rollup decides the verdict; the counts are still summarizeChecks' job.
   const summary = summarizeChecks(checks);
   const checksRollup = status?.checksStatus ?? pr.checksStatus;
-  if (summary.failing > 0) {
+  if (checksRollup === "not_run") {
+    // Ordered ahead of the raw counts on purpose: a `not_run` rollup can
+    // coexist with pending or failing third-party rows, and reporting those
+    // would put a producer-blind number where the honest answer is "nothing
+    // verified this commit".
+    items.push({
+      id: "checks",
+      label: "No CI has run on this commit",
+      state: "neutral",
+    });
+  } else if (summary.failing > 0) {
     items.push({
       id: "checks",
       label: `${summary.failing} failing check${summary.failing === 1 ? "" : "s"}`,
@@ -243,12 +256,6 @@ export function buildMergeChecklist(args: {
       label: `${summary.pending} pending check${summary.pending === 1 ? "" : "s"}`,
       state: "neutral",
     });
-  } else if (checksRollup === "not_run") {
-    items.push({
-      id: "checks",
-      label: "No CI has run on this commit",
-      state: "neutral",
-    });
   } else if (summary.passing > 0) {
     items.push({
       id: "checks",

apps/desktop/src/shared/prChecksRollup.test.ts

@@ -99,7 +99,7 @@ describe("rollupChecks — producer awareness", () => {
     expect(result.status).toBe("not_run");
   });
 
-  it("recognizes only github-actions as a CI app slug", () => {
+  it("excludes known non-CI apps and admits everything else", () => {
     expect(isCiProducerAppSlug("github-actions")).toBe(true);
     expect(isCiProducerAppSlug("GitHub-Actions")).toBe(true);
     expect(isCiProducerAppSlug("vercel")).toBe(false);

apps/desktop/src/shared/prChecksRollup.ts

@@ -306,7 +306,11 @@ export function rollupChecks(input: ChecksRollupInput): ChecksRollup {
       // simply one GitHub has not registered yet. Calling that "not run" made
       // every single push flash a spurious "CI has not run" card before the
       // suite appeared.
-      status: ciProducerCount > 0 || !stale ? "pending" : "not_run",
+      // `hasPass` keeps a partially-verified commit pending while it waits for
+      // the missing context. Bare `ciProducerCount > 0` used to do the same,
+      // which pinned a stale commit whose CI was entirely SKIPPED at `pending`
+      // forever — producers reported, but nothing verified anything.
+      status: hasPass || !stale ? "pending" : "not_run",
       reason: `${pluralize(missingRequiredContexts.length, "required check has", "required checks have")} not reported${stale ? "" : " yet"}: ${formatContexts(missingRequiredContexts)}.`,
       missingRequiredContexts,
     };

apps/ios/ADE/Resources/DatabaseBootstrap.sql

@@ -475,6 +475,10 @@ alter table pull_requests add column head_sha text;
 
 alter table pull_requests add column creation_strategy text;
 alter table pull_requests add column merged_at text;
+-- ADE-135: the desktop rollup replicates these through cr-sqlite, so the column
+-- must exist before a changeset carrying it arrives.
+alter table pull_requests add column checks_reason text;
+alter table pull_requests add column checks_missing_required text;
 
 drop table if exists github_pr_cache;